Re: [openstack-dev] [nova][oslo.utils] Bug-1680130 Check validation of UUID length

[Kekane, Abhishek]

Hi All,

As per suggested by Doug, this is what we will implement.

Add new method (format_canonical_uuid()) in oslo_utils.uuidutils module which 
will return valid uuid and then all other projects which are using 
is_uuid_like() method needs to call this method to get valid uuid. This 
approach sounds reasonable to me. 

Please let me know your opinion about this approach.

Thank you,

Abhishek Kekane

-Original Message-
From: Doug Hellmann [mailto:d...@doughellmann.com] 
Sent: Wednesday, April 26, 2017 9:01 PM
To: openstack-dev
Subject: Re: [openstack-dev] [nova][oslo.utils] Bug-1680130 Check validation of 
UUID length

Excerpts from Sean Dague's message of 2017-04-26 10:55:14 -0400:
> On 04/26/2017 10:47 AM, Doug Hellmann wrote:
> > Excerpts from Sean Dague's message of 2017-04-26 09:01:32 -0400:
> >> On 04/26/2017 08:36 AM, Doug Hellmann wrote:
> >>> Excerpts from Kekane, Abhishek's message of 2017-04-26 07:00:22 +:
> >>>> Hi All,
> >>>>
> >>>> As per suggested by @jay_pipes's
> >>>> if val.count('-') not in (0, 4):
> >>>> raise TypeError
> >>>>
> >>>> It is not sufficient solution because "is_uuid_like" returns only True 
> >>>> or False.
> >>>> For example,
> >>>>
> >>>> If user passes uuid like "urn:----" or 
> >>>> "urn:uuid:----" then "is_uuid_like" 
> >>>> method returns True as it is valid uuid format, but when this uuid tries 
> >>>> to insert into database table then it gives DBDataError because the 
> >>>> reason is in database "block_device_mapping" table has "volume_id" field 
> >>>> of 36 characters only so while inserting data to the table through 
> >>>> 'BlockDeviceMapping' object it raises DBDataError.
> >>>>
> >>>> Doug's solution of adding another method format_canonical_uuid() which 
> >>>> would format it with the proper number of hyphens and return actual UUID 
> >>>> will break backward compatibility IMO. Because of adding this new method 
> >>>> in oslo_utils then we have to make changes in all projects which are 
> >>>> using this is_uuid_like().
> >>>
> >>> I don't understand why adding a new function breaks backwards 
> >>> compatibility. Can you elaborate on why you think so?
> >>
> >> I'm not sure why it's believed it would break compatibility, 
> >> however
> >> format_canonical_uuid() isn't what Nova needs here.
> >>
> >> Nova actually wants to stop bad UUIDs ever getting past our API 
> >> layer, and just spin back to the user that they handed us corrupt 
> >> data. Because it will matter later if they try to use things in 
> >> comparisons. Papering over bad format isn't what we want or intended.
> >>
> >> I think we will end up needing a "is_uuid" which accepts the 
> >> standard dashed format only.
> >>
> >> -Sean
> >>
> > 
> > Sure, that's definitely another option, and again a new function 
> > would be the way to do it and maintain backwards compatibility.
> > 
> > It sounds like there's a chance there's already bad data in the 
> > database, though? For example a UUID presented without the dashes 
> > would have passed the existing check and been able to be stored in 
> > the field because it's shorter than the max length. What happens to 
> > those records?
> 
> That is a good question, and one where we have to figure out what the 
> cost of updating that data would be. I do wonder in what operations 
> that round trips and becomes a good value later.
> 
> But, at a minimum, we want to prevent new bad data from landing.
> 
> -Sean
> 

Maybe preventing writes with bad data, but allowing queries with the existing 
looser constraint, solves the problem? Presumably users querying against this 
field already have to enter the UUID in exactly the same way it was recorded, 
since it's not being converted to a canonical form? Or maybe this is not a 
field used in queries?

Either way, I agree the bad data should be blocked with more strict checks on 
input.

Doug

__
OpenStack Development Mailing List (not for usage questions)
Unsubscribe: openstack-dev-requ...@lists.openstack.org?subject:unsubscribe
http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev

__
Disclaimer: This email and any attachments are sent in strictest confidence
for the sole use of the addressee and may contain legally privileged,
confidential, and proprietary data. If you are not the intended recipient,
please advise the sender by replying promptly to this email and then delete
and destroy this email and any attachments without any further use, copying
or forwarding.
__
OpenStack Development Mailing List (not for usage questions)
Unsubscribe: openstack-dev-requ...@lists.openstack.org?subject:unsubscribe
http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev

Re: [openstack-dev] [nova][oslo.utils] Bug-1680130 Check validation of UUID length

[Doug Hellmann]

Excerpts from Sean Dague's message of 2017-04-26 10:55:14 -0400:
> On 04/26/2017 10:47 AM, Doug Hellmann wrote:
> > Excerpts from Sean Dague's message of 2017-04-26 09:01:32 -0400:
> >> On 04/26/2017 08:36 AM, Doug Hellmann wrote:
> >>> Excerpts from Kekane, Abhishek's message of 2017-04-26 07:00:22 +:
>  Hi All,
> 
>  As per suggested by @jay_pipes's
>  if val.count('-') not in (0, 4):
>  raise TypeError
> 
>  It is not sufficient solution because "is_uuid_like" returns only True 
>  or False.
>  For example,
> 
>  If user passes uuid like "urn:----" or 
>  "urn:uuid:----" then "is_uuid_like" 
>  method returns True as it is valid uuid format, but when this uuid tries 
>  to insert into database table then it gives DBDataError because the 
>  reason is in database "block_device_mapping" table has "volume_id" field 
>  of 36 characters only so while inserting data to the table through 
>  'BlockDeviceMapping' object it raises DBDataError.
> 
>  Doug's solution of adding another method format_canonical_uuid() which 
>  would format it with the proper number of hyphens and return actual UUID 
>  will break backward compatibility IMO. Because of adding this new method 
>  in oslo_utils then we have to make changes in all projects which are 
>  using this is_uuid_like().
> >>>
> >>> I don't understand why adding a new function breaks backwards
> >>> compatibility. Can you elaborate on why you think so?
> >>
> >> I'm not sure why it's believed it would break compatibility, however
> >> format_canonical_uuid() isn't what Nova needs here.
> >>
> >> Nova actually wants to stop bad UUIDs ever getting past our API layer,
> >> and just spin back to the user that they handed us corrupt data. Because
> >> it will matter later if they try to use things in comparisons. Papering
> >> over bad format isn't what we want or intended.
> >>
> >> I think we will end up needing a "is_uuid" which accepts the standard
> >> dashed format only.
> >>
> >> -Sean
> >>
> > 
> > Sure, that's definitely another option, and again a new function
> > would be the way to do it and maintain backwards compatibility.
> > 
> > It sounds like there's a chance there's already bad data in the
> > database, though? For example a UUID presented without the dashes
> > would have passed the existing check and been able to be stored in
> > the field because it's shorter than the max length. What happens
> > to those records?
> 
> That is a good question, and one where we have to figure out what the
> cost of updating that data would be. I do wonder in what operations that
> round trips and becomes a good value later.
> 
> But, at a minimum, we want to prevent new bad data from landing.
> 
> -Sean
> 

Maybe preventing writes with bad data, but allowing queries with the
existing looser constraint, solves the problem? Presumably users
querying against this field already have to enter the UUID in exactly
the same way it was recorded, since it's not being converted to a
canonical form? Or maybe this is not a field used in queries?

Either way, I agree the bad data should be blocked with more strict
checks on input.

Doug

__
OpenStack Development Mailing List (not for usage questions)
Unsubscribe: openstack-dev-requ...@lists.openstack.org?subject:unsubscribe
http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev

Re: [openstack-dev] [nova][oslo.utils] Bug-1680130 Check validation of UUID length

[Sean Dague]

On 04/26/2017 10:47 AM, Doug Hellmann wrote:
> Excerpts from Sean Dague's message of 2017-04-26 09:01:32 -0400:
>> On 04/26/2017 08:36 AM, Doug Hellmann wrote:
>>> Excerpts from Kekane, Abhishek's message of 2017-04-26 07:00:22 +:
 Hi All,

 As per suggested by @jay_pipes's
 if val.count('-') not in (0, 4):
 raise TypeError

 It is not sufficient solution because "is_uuid_like" returns only True or 
 False.
 For example,

 If user passes uuid like "urn:----" or 
 "urn:uuid:----" then "is_uuid_like" method 
 returns True as it is valid uuid format, but when this uuid tries to 
 insert into database table then it gives DBDataError because the reason is 
 in database "block_device_mapping" table has "volume_id" field of 36 
 characters only so while inserting data to the table through 
 'BlockDeviceMapping' object it raises DBDataError.

 Doug's solution of adding another method format_canonical_uuid() which 
 would format it with the proper number of hyphens and return actual UUID 
 will break backward compatibility IMO. Because of adding this new method 
 in oslo_utils then we have to make changes in all projects which are using 
 this is_uuid_like().
>>>
>>> I don't understand why adding a new function breaks backwards
>>> compatibility. Can you elaborate on why you think so?
>>
>> I'm not sure why it's believed it would break compatibility, however
>> format_canonical_uuid() isn't what Nova needs here.
>>
>> Nova actually wants to stop bad UUIDs ever getting past our API layer,
>> and just spin back to the user that they handed us corrupt data. Because
>> it will matter later if they try to use things in comparisons. Papering
>> over bad format isn't what we want or intended.
>>
>> I think we will end up needing a "is_uuid" which accepts the standard
>> dashed format only.
>>
>> -Sean
>>
> 
> Sure, that's definitely another option, and again a new function
> would be the way to do it and maintain backwards compatibility.
> 
> It sounds like there's a chance there's already bad data in the
> database, though? For example a UUID presented without the dashes
> would have passed the existing check and been able to be stored in
> the field because it's shorter than the max length. What happens
> to those records?

That is a good question, and one where we have to figure out what the
cost of updating that data would be. I do wonder in what operations that
round trips and becomes a good value later.

But, at a minimum, we want to prevent new bad data from landing.

-Sean

-- 
Sean Dague
http://dague.net

__
OpenStack Development Mailing List (not for usage questions)
Unsubscribe: openstack-dev-requ...@lists.openstack.org?subject:unsubscribe
http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev

Re: [openstack-dev] [nova][oslo.utils] Bug-1680130 Check validation of UUID length

[Doug Hellmann]

Excerpts from Sean Dague's message of 2017-04-26 09:01:32 -0400:
> On 04/26/2017 08:36 AM, Doug Hellmann wrote:
> > Excerpts from Kekane, Abhishek's message of 2017-04-26 07:00:22 +:
> >> Hi All,
> >>
> >> As per suggested by @jay_pipes's
> >> if val.count('-') not in (0, 4):
> >> raise TypeError
> >>
> >> It is not sufficient solution because "is_uuid_like" returns only True or 
> >> False.
> >> For example,
> >>
> >> If user passes uuid like "urn:----" or 
> >> "urn:uuid:----" then "is_uuid_like" method 
> >> returns True as it is valid uuid format, but when this uuid tries to 
> >> insert into database table then it gives DBDataError because the reason is 
> >> in database "block_device_mapping" table has "volume_id" field of 36 
> >> characters only so while inserting data to the table through 
> >> 'BlockDeviceMapping' object it raises DBDataError.
> >>
> >> Doug's solution of adding another method format_canonical_uuid() which 
> >> would format it with the proper number of hyphens and return actual UUID 
> >> will break backward compatibility IMO. Because of adding this new method 
> >> in oslo_utils then we have to make changes in all projects which are using 
> >> this is_uuid_like().
> > 
> > I don't understand why adding a new function breaks backwards
> > compatibility. Can you elaborate on why you think so?
> 
> I'm not sure why it's believed it would break compatibility, however
> format_canonical_uuid() isn't what Nova needs here.
> 
> Nova actually wants to stop bad UUIDs ever getting past our API layer,
> and just spin back to the user that they handed us corrupt data. Because
> it will matter later if they try to use things in comparisons. Papering
> over bad format isn't what we want or intended.
> 
> I think we will end up needing a "is_uuid" which accepts the standard
> dashed format only.
> 
> -Sean
> 

Sure, that's definitely another option, and again a new function
would be the way to do it and maintain backwards compatibility.

It sounds like there's a chance there's already bad data in the
database, though? For example a UUID presented without the dashes
would have passed the existing check and been able to be stored in
the field because it's shorter than the max length. What happens
to those records?

Doug

__
OpenStack Development Mailing List (not for usage questions)
Unsubscribe: openstack-dev-requ...@lists.openstack.org?subject:unsubscribe
http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev

Re: [openstack-dev] [nova][oslo.utils] Bug-1680130 Check validation of UUID length

[Sean Dague]

On 04/26/2017 08:36 AM, Doug Hellmann wrote:
> Excerpts from Kekane, Abhishek's message of 2017-04-26 07:00:22 +:
>> Hi All,
>>
>> As per suggested by @jay_pipes's
>> if val.count('-') not in (0, 4):
>> raise TypeError
>>
>> It is not sufficient solution because "is_uuid_like" returns only True or 
>> False.
>> For example,
>>
>> If user passes uuid like "urn:----" or 
>> "urn:uuid:----" then "is_uuid_like" method 
>> returns True as it is valid uuid format, but when this uuid tries to insert 
>> into database table then it gives DBDataError because the reason is in 
>> database "block_device_mapping" table has "volume_id" field of 36 characters 
>> only so while inserting data to the table through 'BlockDeviceMapping' 
>> object it raises DBDataError.
>>
>> Doug's solution of adding another method format_canonical_uuid() which would 
>> format it with the proper number of hyphens and return actual UUID will 
>> break backward compatibility IMO. Because of adding this new method in 
>> oslo_utils then we have to make changes in all projects which are using this 
>> is_uuid_like().
> 
> I don't understand why adding a new function breaks backwards
> compatibility. Can you elaborate on why you think so?

I'm not sure why it's believed it would break compatibility, however
format_canonical_uuid() isn't what Nova needs here.

Nova actually wants to stop bad UUIDs ever getting past our API layer,
and just spin back to the user that they handed us corrupt data. Because
it will matter later if they try to use things in comparisons. Papering
over bad format isn't what we want or intended.

I think we will end up needing a "is_uuid" which accepts the standard
dashed format only.

-Sean

-- 
Sean Dague
http://dague.net

__
OpenStack Development Mailing List (not for usage questions)
Unsubscribe: openstack-dev-requ...@lists.openstack.org?subject:unsubscribe
http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev

Re: [openstack-dev] [nova][oslo.utils] Bug-1680130 Check validation of UUID length

[Doug Hellmann]

Excerpts from Kekane, Abhishek's message of 2017-04-26 07:00:22 +:
> Hi All,
> 
> As per suggested by @jay_pipes's
> if val.count('-') not in (0, 4):
> raise TypeError
> 
> It is not sufficient solution because "is_uuid_like" returns only True or 
> False.
> For example,
> 
> If user passes uuid like "urn:----" or 
> "urn:uuid:----" then "is_uuid_like" method 
> returns True as it is valid uuid format, but when this uuid tries to insert 
> into database table then it gives DBDataError because the reason is in 
> database "block_device_mapping" table has "volume_id" field of 36 characters 
> only so while inserting data to the table through 'BlockDeviceMapping' object 
> it raises DBDataError.
> 
> Doug's solution of adding another method format_canonical_uuid() which would 
> format it with the proper number of hyphens and return actual UUID will break 
> backward compatibility IMO. Because of adding this new method in oslo_utils 
> then we have to make changes in all projects which are using this 
> is_uuid_like().

I don't understand why adding a new function breaks backwards
compatibility. Can you elaborate on why you think so?

Doug

> 
> Please let me know if you have any suggestions on the same, IMO restricting 
> this uuid size at schema level is one solution but not all projects supports 
> schema validation.
> 
> Thank you,
> 
> Abhishek
> 
> 
> From: Lance Bragstad [mailto:lbrags...@gmail.com]
> Sent: Monday, April 24, 2017 11:50 PM
> To: OpenStack Development Mailing List (not for usage questions)
> Subject: Re: [openstack-dev] [nova][oslo.utils] Bug-1680130 Check validation 
> of UUID length
> 
> We had to do similar things in keystone in order to validate uuid-ish types 
> (just not as fancy) [0] [1]. If we didn't have to worry about being backwards 
> compatible with non-uuid formats, it would be awesome to have one 
> implementation for checking that.
> 
> [0] 
> https://github.com/openstack/keystone/blob/6c6589d2b0f308cb788b37b29ebde515304ee41e/keystone/identity/schema.py#L69
> [1] 
> https://github.com/openstack/keystone/blob/6c6589d2b0f308cb788b37b29ebde515304ee41e/keystone/common/validation/parameter_types.py#L38-L45
> 
> On Mon, Apr 24, 2017 at 1:05 PM, Matt Riedemann 
> <mriede...@gmail.com<mailto:mriede...@gmail.com>> wrote:
> On 4/24/2017 12:58 PM, Sean Dague wrote:
> 
> Which uses is_uuid_like to do the validation -
> https://github.com/openstack/nova/blob/1106477b78c80743e6443abc30911b24a9ab7b15/nova/api/validation/validators.py#L85-L87
> 
> We assumed (as did many others) that is_uuid_like was strict enough for
> param validation. It is apparently not.
> 
> Either it needs to be fixed to be so, or some other function needs to be
> created that is, that people can cut over to.
> 
> -Sean
> 
> Well kiss my grits. I had always assumed that was built into jsonschema.
> 
> --
> 
> Thanks,
> 
> Matt
> 

__
OpenStack Development Mailing List (not for usage questions)
Unsubscribe: openstack-dev-requ...@lists.openstack.org?subject:unsubscribe
http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev

Re: [openstack-dev] [nova][oslo.utils] Bug-1680130 Check validation of UUID length

[Kekane, Abhishek]

Hi All,

As per suggested by @jay_pipes's
if val.count('-') not in (0, 4):
raise TypeError

It is not sufficient solution because "is_uuid_like" returns only True or False.
For example,

If user passes uuid like "urn:----" or 
"urn:uuid:----" then "is_uuid_like" method 
returns True as it is valid uuid format, but when this uuid tries to insert 
into database table then it gives DBDataError because the reason is in database 
"block_device_mapping" table has "volume_id" field of 36 characters only so 
while inserting data to the table through 'BlockDeviceMapping' object it raises 
DBDataError.

Doug's solution of adding another method format_canonical_uuid() which would 
format it with the proper number of hyphens and return actual UUID will break 
backward compatibility IMO. Because of adding this new method in oslo_utils 
then we have to make changes in all projects which are using this 
is_uuid_like().

Please let me know if you have any suggestions on the same, IMO restricting 
this uuid size at schema level is one solution but not all projects supports 
schema validation.

Thank you,

Abhishek


From: Lance Bragstad [mailto:lbrags...@gmail.com]
Sent: Monday, April 24, 2017 11:50 PM
To: OpenStack Development Mailing List (not for usage questions)
Subject: Re: [openstack-dev] [nova][oslo.utils] Bug-1680130 Check validation of 
UUID length

We had to do similar things in keystone in order to validate uuid-ish types 
(just not as fancy) [0] [1]. If we didn't have to worry about being backwards 
compatible with non-uuid formats, it would be awesome to have one 
implementation for checking that.

[0] 
https://github.com/openstack/keystone/blob/6c6589d2b0f308cb788b37b29ebde515304ee41e/keystone/identity/schema.py#L69
[1] 
https://github.com/openstack/keystone/blob/6c6589d2b0f308cb788b37b29ebde515304ee41e/keystone/common/validation/parameter_types.py#L38-L45

On Mon, Apr 24, 2017 at 1:05 PM, Matt Riedemann 
<mriede...@gmail.com<mailto:mriede...@gmail.com>> wrote:
On 4/24/2017 12:58 PM, Sean Dague wrote:

Which uses is_uuid_like to do the validation -
https://github.com/openstack/nova/blob/1106477b78c80743e6443abc30911b24a9ab7b15/nova/api/validation/validators.py#L85-L87

We assumed (as did many others) that is_uuid_like was strict enough for
param validation. It is apparently not.

Either it needs to be fixed to be so, or some other function needs to be
created that is, that people can cut over to.

-Sean

Well kiss my grits. I had always assumed that was built into jsonschema.

--

Thanks,

Matt


__
OpenStack Development Mailing List (not for usage questions)
Unsubscribe: 
openstack-dev-requ...@lists.openstack.org?subject:unsubscribe<http://openstack-dev-requ...@lists.openstack.org?subject:unsubscribe>
http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev


__
Disclaimer: This email and any attachments are sent in strictest confidence
for the sole use of the addressee and may contain legally privileged,
confidential, and proprietary data. If you are not the intended recipient,
please advise the sender by replying promptly to this email and then delete
and destroy this email and any attachments without any further use, copying
or forwarding.
__
OpenStack Development Mailing List (not for usage questions)
Unsubscribe: openstack-dev-requ...@lists.openstack.org?subject:unsubscribe
http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev

Re: [openstack-dev] [nova][oslo.utils] Bug-1680130 Check validation of UUID length

[Lance Bragstad]

We had to do similar things in keystone in order to validate uuid-ish types
(just not as fancy) [0] [1]. If we didn't have to worry about being
backwards compatible with non-uuid formats, it would be awesome to have one
implementation for checking that.

[0]
https://github.com/openstack/keystone/blob/6c6589d2b0f308cb788b37b29ebde515304ee41e/keystone/identity/schema.py#L69
[1]
https://github.com/openstack/keystone/blob/6c6589d2b0f308cb788b37b29ebde515304ee41e/keystone/common/validation/parameter_types.py#L38-L45

On Mon, Apr 24, 2017 at 1:05 PM, Matt Riedemann  wrote:

> On 4/24/2017 12:58 PM, Sean Dague wrote:
>
>>
>> Which uses is_uuid_like to do the validation -
>> https://github.com/openstack/nova/blob/1106477b78c80743e6443
>> abc30911b24a9ab7b15/nova/api/validation/validators.py#L85-L87
>>
>> We assumed (as did many others) that is_uuid_like was strict enough for
>> param validation. It is apparently not.
>>
>> Either it needs to be fixed to be so, or some other function needs to be
>> created that is, that people can cut over to.
>>
>> -Sean
>>
>>
> Well kiss my grits. I had always assumed that was built into jsonschema.
>
> --
>
> Thanks,
>
> Matt
>
>
> __
> OpenStack Development Mailing List (not for usage questions)
> Unsubscribe: openstack-dev-requ...@lists.openstack.org?subject:unsubscribe
> http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
>
__
OpenStack Development Mailing List (not for usage questions)
Unsubscribe: openstack-dev-requ...@lists.openstack.org?subject:unsubscribe
http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev

Re: [openstack-dev] [nova][oslo.utils] Bug-1680130 Check validation of UUID length

[Sean Dague]

On 04/24/2017 01:58 PM, Sean Dague wrote:
> On 04/24/2017 12:23 PM, Matt Riedemann wrote:
> 
>>
>> Is "-----" actually getting past the
>> jsonschema validation check when attaching a volume to a server? Because
>> that's looking for a uuid:
>>
>> https://github.com/openstack/nova/blob/0039231719d2a02c59e7cd76631e2ff03cc86b0d/nova/api/validation/parameter_types.py#L298
> 
> Which uses is_uuid_like to do the validation -
> https://github.com/openstack/nova/blob/1106477b78c80743e6443abc30911b24a9ab7b15/nova/api/validation/validators.py#L85-L87
> 
> We assumed (as did many others) that is_uuid_like was strict enough for
> param validation. It is apparently not.
> 
> Either it needs to be fixed to be so, or some other function needs to be
> created that is, that people can cut over to.

And given the doc string -
https://docs.openstack.org/developer/oslo.utils/api/uuidutils.html#oslo_utils.uuidutils.is_uuid_like
it's easy to understand why this mistake was made by so many.

-Sean

-- 
Sean Dague
http://dague.net

__
OpenStack Development Mailing List (not for usage questions)
Unsubscribe: openstack-dev-requ...@lists.openstack.org?subject:unsubscribe
http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev

Re: [openstack-dev] [nova][oslo.utils] Bug-1680130 Check validation of UUID length

[Matt Riedemann]

On 4/24/2017 12:58 PM, Sean Dague wrote:


Which uses is_uuid_like to do the validation -
https://github.com/openstack/nova/blob/1106477b78c80743e6443abc30911b24a9ab7b15/nova/api/validation/validators.py#L85-L87

We assumed (as did many others) that is_uuid_like was strict enough for
param validation. It is apparently not.

Either it needs to be fixed to be so, or some other function needs to be
created that is, that people can cut over to.

-Sean



Well kiss my grits. I had always assumed that was built into jsonschema.

--

Thanks,

Matt

__
OpenStack Development Mailing List (not for usage questions)
Unsubscribe: openstack-dev-requ...@lists.openstack.org?subject:unsubscribe
http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev

Re: [openstack-dev] [nova][oslo.utils] Bug-1680130 Check validation of UUID length

[Sean Dague]

On 04/24/2017 12:23 PM, Matt Riedemann wrote:

> 
> Is "-----" actually getting past the
> jsonschema validation check when attaching a volume to a server? Because
> that's looking for a uuid:
> 
> https://github.com/openstack/nova/blob/0039231719d2a02c59e7cd76631e2ff03cc86b0d/nova/api/validation/parameter_types.py#L298

Which uses is_uuid_like to do the validation -
https://github.com/openstack/nova/blob/1106477b78c80743e6443abc30911b24a9ab7b15/nova/api/validation/validators.py#L85-L87

We assumed (as did many others) that is_uuid_like was strict enough for
param validation. It is apparently not.

Either it needs to be fixed to be so, or some other function needs to be
created that is, that people can cut over to.

-Sean

-- 
Sean Dague
http://dague.net

__
OpenStack Development Mailing List (not for usage questions)
Unsubscribe: openstack-dev-requ...@lists.openstack.org?subject:unsubscribe
http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev

Re: [openstack-dev] [nova][oslo.utils] Bug-1680130 Check validation of UUID length

[Matt Riedemann]

On 4/24/2017 8:45 AM, Jadhav, Pooja wrote:

Hi Devs,



I want your opinion about bug: https://bugs.launchpad.net/nova/+bug/1680130



When user passes incorrect formatted UUID, volume UUID like:
-----(please note double hyphen) for
attaching a volume to an instance using "volume-attach" API then it
results into DBDataError with following error message: "Data too long
for column 'volume_id'". The reason is in database
"block_device_mapping" table has "volume_id" field of 36 characters only
so while inserting data to the table through 'BlockDeviceMapping' object
it raises DBDaTaError.



In current code, volume_id is of ‘UUID’ format so it uses
"is_uuid_like"[4] method of oslo_utils and in this method, it removes
all the hyphens and checks 32 length UUID and returns true or false. As
"-----" this UUID treated as valid and
further it goes into database table for insertion, as its size is more
than 36 characters it gives DBDataError.



There are various solutions we can apply to validate volume UUID in this
case:



Solution 1:

We can restrict the length of volume UUID using maxlength property in
schema validation.



Advantage:

This solution is better than solution 2 and 3 as we can restrict the
invalid UUID at schema [1] level itself by adding 'maxLength'[2].




Solution 2:

Before creating a volume BDM object, we can check that the provided
volume is actually present or not.



Advantage:

Volume BDM creation can be avoided if the volume does not
exists.



Disadvantage:

IMO this solution is not better because we need to change the current
code. Because in the current code after creating volume BDM it is
checking volume is exists or not.

We have to check volume existence before creating volume BDM object. For
that we need to modify the "_check_attach_and_reserve_volume" method
[3]. But this method get used at 3 places. According to it, we have to
modify all the occurrences as per behavior.



Solution 3:

We can check UUID in central place means in "is_uuid_like" method of
oslo_utils [4].



Advantage:

If we change the "is_uuid_like" method then same issue might be solved
for the rest of the APIs.




Disadvantage:

IMO this also not a better solution because if we change the
"is_uuid_like" method then it will affect on several different projects.



Please let me know your opinion for the
same.



[1]
https://github.com/openstack/nova/blob/master/nova/api/openstack/compute/schemas/volumes.py#L65



[2]
https://github.com/openstack/nova/blob/master/nova/api/validation/parameter_types.py#L297



[3] https://github.com/openstack/nova/blob/master/nova/compute/api.py#L3721



[4]
https://github.com/openstack/oslo.utils/blob/master/oslo_utils/uuidutils.py#L45






__
Disclaimer: This email and any attachments are sent in strictest confidence
for the sole use of the addressee and may contain legally privileged,
confidential, and proprietary data. If you are not the intended recipient,
please advise the sender by replying promptly to this email and then delete
and destroy this email and any attachments without any further use, copying
or forwarding.


__
OpenStack Development Mailing List (not for usage questions)
Unsubscribe: openstack-dev-requ...@lists.openstack.org?subject:unsubscribe
http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev



Is "-----" actually getting past the 
jsonschema validation check when attaching a volume to a server? Because 
that's looking for a uuid:


https://github.com/openstack/nova/blob/0039231719d2a02c59e7cd76631e2ff03cc86b0d/nova/api/validation/parameter_types.py#L298

--

Thanks,

Matt

__
OpenStack Development Mailing List (not for usage questions)
Unsubscribe: openstack-dev-requ...@lists.openstack.org?subject:unsubscribe
http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev

Re: [openstack-dev] [nova][oslo.utils] Bug-1680130 Check validation of UUID length

[Jay Pipes]

On 04/24/2017 11:00 AM, Eric Fried wrote:

That's not the only way you can break this, though.  For example,
'12-3-45-6-78-12-3456-781-234-56-781-234-56-79' still passes the
modified is_uuid_like(), but still manifests the bug.

Trying to get is_uuid_like() to cover all possible formatting snafus
while still allowing the same formats as before (e.g. without any
hyphens at all) is a rabbit hole of mystical depths.


Not necessarily a rabbit hole of mystical depths. :)

We only care about hyphens. So, we could have this check instead:

if val.count('-') not in (0, 4):
raise TypeError

Best,
-jay


On 04/24/2017 09:44 AM, Jay Pipes wrote:

On 04/24/2017 09:45 AM, Jadhav, Pooja wrote:

Solution 3:

We can check UUID in central place means in "is_uuid_like" method of
oslo_utils [4].


This gets my vote. It's a bug in the is_uuid_like() function, IMHO, that
is returns True for badly-formatted UUID values (like having two
consecutive hyphens).

FTR, the fix would be pretty simple. Just change this [1] line from this:

return str(uuid.UUID(val)).replace('-', '') == _format_uuid_string(val)

to this:

# Disallow two consecutive hyphens
if '--' in val:
raise TypeError
return str(uuid.UUID(val)).replace('-', '') == _format_uuid_string(val)

Fix it there and you fix this issue for all projects that use it.

Best,
-jay

[1]
https://github.com/openstack/oslo.utils/blob/master/oslo_utils/uuidutils.py#L56


__
OpenStack Development Mailing List (not for usage questions)
Unsubscribe: openstack-dev-requ...@lists.openstack.org?subject:unsubscribe
http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev


__
OpenStack Development Mailing List (not for usage questions)
Unsubscribe: openstack-dev-requ...@lists.openstack.org?subject:unsubscribe
http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev



__
OpenStack Development Mailing List (not for usage questions)
Unsubscribe: openstack-dev-requ...@lists.openstack.org?subject:unsubscribe
http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev

Re: [openstack-dev] [nova][oslo.utils] Bug-1680130 Check validation of UUID length

[Doug Hellmann]

Excerpts from Jay Pipes's message of 2017-04-24 10:44:47 -0400:
> On 04/24/2017 09:45 AM, Jadhav, Pooja wrote:
> > Solution 3:
> >
> > We can check UUID in central place means in "is_uuid_like" method of
> > oslo_utils [4].
> 
> This gets my vote. It's a bug in the is_uuid_like() function, IMHO, that 
> is returns True for badly-formatted UUID values (like having two 
> consecutive hyphens).
> 
> FTR, the fix would be pretty simple. Just change this [1] line from this:
> 
> return str(uuid.UUID(val)).replace('-', '') == _format_uuid_string(val)
> 
> to this:
> 
> # Disallow two consecutive hyphens
> if '--' in val:
>  raise TypeError
> return str(uuid.UUID(val)).replace('-', '') == _format_uuid_string(val)
> 
> Fix it there and you fix this issue for all projects that use it.
> 
> Best,
> -jay
> 
> [1] 
> https://github.com/openstack/oslo.utils/blob/master/oslo_utils/uuidutils.py#L56
> 

I think the point of that function was to be a little forgiving of
typos, since we use UUIDs so much in the command line interfaces.

Doug

__
OpenStack Development Mailing List (not for usage questions)
Unsubscribe: openstack-dev-requ...@lists.openstack.org?subject:unsubscribe
http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev

Re: [openstack-dev] [nova][oslo.utils] Bug-1680130 Check validation of UUID length

[Eric Fried]

That's not the only way you can break this, though.  For example,
'12-3-45-6-78-12-3456-781-234-56-781-234-56-79' still passes the
modified is_uuid_like(), but still manifests the bug.

Trying to get is_uuid_like() to cover all possible formatting snafus
while still allowing the same formats as before (e.g. without any
hyphens at all) is a rabbit hole of mystical depths.

On 04/24/2017 09:44 AM, Jay Pipes wrote:
> On 04/24/2017 09:45 AM, Jadhav, Pooja wrote:
>> Solution 3:
>>
>> We can check UUID in central place means in "is_uuid_like" method of
>> oslo_utils [4].
> 
> This gets my vote. It's a bug in the is_uuid_like() function, IMHO, that
> is returns True for badly-formatted UUID values (like having two
> consecutive hyphens).
> 
> FTR, the fix would be pretty simple. Just change this [1] line from this:
> 
> return str(uuid.UUID(val)).replace('-', '') == _format_uuid_string(val)
> 
> to this:
> 
> # Disallow two consecutive hyphens
> if '--' in val:
> raise TypeError
> return str(uuid.UUID(val)).replace('-', '') == _format_uuid_string(val)
> 
> Fix it there and you fix this issue for all projects that use it.
> 
> Best,
> -jay
> 
> [1]
> https://github.com/openstack/oslo.utils/blob/master/oslo_utils/uuidutils.py#L56
> 
> 
> __
> OpenStack Development Mailing List (not for usage questions)
> Unsubscribe: openstack-dev-requ...@lists.openstack.org?subject:unsubscribe
> http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev

__
OpenStack Development Mailing List (not for usage questions)
Unsubscribe: openstack-dev-requ...@lists.openstack.org?subject:unsubscribe
http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev

Re: [openstack-dev] [nova][oslo.utils] Bug-1680130 Check validation of UUID length

[Jay Pipes]

On 04/24/2017 09:45 AM, Jadhav, Pooja wrote:

Solution 3:

We can check UUID in central place means in "is_uuid_like" method of
oslo_utils [4].


This gets my vote. It's a bug in the is_uuid_like() function, IMHO, that 
is returns True for badly-formatted UUID values (like having two 
consecutive hyphens).


FTR, the fix would be pretty simple. Just change this [1] line from this:

return str(uuid.UUID(val)).replace('-', '') == _format_uuid_string(val)

to this:

# Disallow two consecutive hyphens
if '--' in val:
raise TypeError
return str(uuid.UUID(val)).replace('-', '') == _format_uuid_string(val)

Fix it there and you fix this issue for all projects that use it.

Best,
-jay

[1] 
https://github.com/openstack/oslo.utils/blob/master/oslo_utils/uuidutils.py#L56


__
OpenStack Development Mailing List (not for usage questions)
Unsubscribe: openstack-dev-requ...@lists.openstack.org?subject:unsubscribe
http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev

Re: [openstack-dev] [nova][oslo.utils] Bug-1680130 Check validation of UUID length

[Eric Fried]

+1.  Provide a sanitize_uuid() or similar, which may be as simple as:

def sanitize_uuid(val):
try:
return uuid.UUID(val)
except ValueError:
raise SomePossiblyNewException(...)

UUID consumers are encouraged, but not required, to use it - so we
retain backward compatibility overall, and fixes like this one can be
implemented individually.

On 04/24/2017 08:53 AM, Doug Hellmann wrote:
> Excerpts from Jadhav, Pooja's message of 2017-04-24 13:45:07 +:
>> Hi Devs,
>>
>> I want your opinion about bug: https://bugs.launchpad.net/nova/+bug/1680130
>>
>> When user passes incorrect formatted UUID, volume UUID like: 
>> -----(please note double hyphen) for 
>> attaching a volume to an instance using "volume-attach" API then it results 
>> into DBDataError with following error message: "Data too long for column 
>> 'volume_id'". The reason is in database "block_device_mapping" table has 
>> "volume_id" field of 36 characters only so while inserting data to the table 
>> through 'BlockDeviceMapping' object it raises DBDaTaError.
>>
>> In current code, volume_id is of 'UUID' format so it uses "is_uuid_like"[4] 
>> method of oslo_utils and in this method, it removes all the hyphens and 
>> checks 32 length UUID and returns true or false. As 
>> "-----" this UUID treated as valid and 
>> further it goes into database table for insertion, as its size is more than 
>> 36 characters it gives DBDataError.
>>
>> There are various solutions we can apply to validate volume UUID in this 
>> case:
>>
>> Solution 1:
>> We can restrict the length of volume UUID using maxlength property in schema 
>> validation.
>>
>> Advantage:
>> This solution is better than solution 2 and 3 as we can restrict the invalid 
>> UUID at schema [1] level itself by adding 'maxLength'[2].
>>
>> Solution 2:
>> Before creating a volume BDM object, we can check that the provided volume 
>> is actually present or not.
>>
>> Advantage:
>> Volume BDM creation can be avoided if the volume does not exists.
>>
>> Disadvantage:
>> IMO this solution is not better because we need to change the current code. 
>> Because in the current code after creating volume BDM it is checking volume 
>> is exists or not.
>> We have to check volume existence before creating volume BDM object. For 
>> that we need to modify the "_check_attach_and_reserve_volume" method [3]. 
>> But this method get used at 3 places. According to it, we have to modify all 
>> the occurrences as per behavior.
>>
>> Solution 3:
>> We can check UUID in central place means in "is_uuid_like" method of 
>> oslo_utils [4].
>>
>> Advantage:
>> If we change the "is_uuid_like" method then same issue might be solved for 
>> the rest of the APIs.
>>
>> Disadvantage:
>> IMO this also not a better solution because if we change the "is_uuid_like" 
>> method then it will affect on several different projects.
> 
> Another option would be to convert the input value to a canonical form.
> So if is_uuid_like() returns true, then pass the value to a new function
> format_canonical_uuid() which would format it with the proper number of
> hyphens. That value could then be stored correctly.
> 
> Doug
> 
>>
>> Please let me know your opinion for the same.
>>
>> [1] 
>> https://github.com/openstack/nova/blob/master/nova/api/openstack/compute/schemas/volumes.py#L65
>>
>> [2] 
>> https://github.com/openstack/nova/blob/master/nova/api/validation/parameter_types.py#L297
>>
>> [3] https://github.com/openstack/nova/blob/master/nova/compute/api.py#L3721
>>
>> [4] 
>> https://github.com/openstack/oslo.utils/blob/master/oslo_utils/uuidutils.py#L45
>>
> 
> __
> OpenStack Development Mailing List (not for usage questions)
> Unsubscribe: openstack-dev-requ...@lists.openstack.org?subject:unsubscribe
> http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
> 

__
OpenStack Development Mailing List (not for usage questions)
Unsubscribe: openstack-dev-requ...@lists.openstack.org?subject:unsubscribe
http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev

Re: [openstack-dev] [nova][oslo.utils] Bug-1680130 Check validation of UUID length

[Doug Hellmann]

Excerpts from Jadhav, Pooja's message of 2017-04-24 13:45:07 +:
> Hi Devs,
> 
> I want your opinion about bug: https://bugs.launchpad.net/nova/+bug/1680130
> 
> When user passes incorrect formatted UUID, volume UUID like: 
> -----(please note double hyphen) for 
> attaching a volume to an instance using "volume-attach" API then it results 
> into DBDataError with following error message: "Data too long for column 
> 'volume_id'". The reason is in database "block_device_mapping" table has 
> "volume_id" field of 36 characters only so while inserting data to the table 
> through 'BlockDeviceMapping' object it raises DBDaTaError.
> 
> In current code, volume_id is of 'UUID' format so it uses "is_uuid_like"[4] 
> method of oslo_utils and in this method, it removes all the hyphens and 
> checks 32 length UUID and returns true or false. As 
> "-----" this UUID treated as valid and 
> further it goes into database table for insertion, as its size is more than 
> 36 characters it gives DBDataError.
> 
> There are various solutions we can apply to validate volume UUID in this case:
> 
> Solution 1:
> We can restrict the length of volume UUID using maxlength property in schema 
> validation.
> 
> Advantage:
> This solution is better than solution 2 and 3 as we can restrict the invalid 
> UUID at schema [1] level itself by adding 'maxLength'[2].
> 
> Solution 2:
> Before creating a volume BDM object, we can check that the provided volume is 
> actually present or not.
> 
> Advantage:
> Volume BDM creation can be avoided if the volume does not exists.
> 
> Disadvantage:
> IMO this solution is not better because we need to change the current code. 
> Because in the current code after creating volume BDM it is checking volume 
> is exists or not.
> We have to check volume existence before creating volume BDM object. For that 
> we need to modify the "_check_attach_and_reserve_volume" method [3]. But this 
> method get used at 3 places. According to it, we have to modify all the 
> occurrences as per behavior.
> 
> Solution 3:
> We can check UUID in central place means in "is_uuid_like" method of 
> oslo_utils [4].
> 
> Advantage:
> If we change the "is_uuid_like" method then same issue might be solved for 
> the rest of the APIs.
> 
> Disadvantage:
> IMO this also not a better solution because if we change the "is_uuid_like" 
> method then it will affect on several different projects.

Another option would be to convert the input value to a canonical form.
So if is_uuid_like() returns true, then pass the value to a new function
format_canonical_uuid() which would format it with the proper number of
hyphens. That value could then be stored correctly.

Doug

> 
> Please let me know your opinion for the same.
> 
> [1] 
> https://github.com/openstack/nova/blob/master/nova/api/openstack/compute/schemas/volumes.py#L65
> 
> [2] 
> https://github.com/openstack/nova/blob/master/nova/api/validation/parameter_types.py#L297
> 
> [3] https://github.com/openstack/nova/blob/master/nova/compute/api.py#L3721
> 
> [4] 
> https://github.com/openstack/oslo.utils/blob/master/oslo_utils/uuidutils.py#L45
> 

__
OpenStack Development Mailing List (not for usage questions)
Unsubscribe: openstack-dev-requ...@lists.openstack.org?subject:unsubscribe
http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev