Do not store headers as None with swift

[Chmouel Boudjnah]

By default when you have an empty string for a value the properties class would set this as None, for swiftclient if we do that it would just pass the info as is and actually set the value of container as None :

stack@devstack:heat/heat/engine$ swift stat testswift-SwiftContainer-fhav4mkpktgx *[master]
       Account: AUTH_4f904e80cacd4f258f7e06ef5bdc1da5
     Container: testswift-SwiftContainer-fhav4mkpktgx
       Objects: 0
         Bytes: 0
      Read ACL: None
     Write ACL: None
       Sync To:
      Sync Key:
 Accept-Ranges: bytes
   X-Timestamp: 1386684773.54007
    X-Trans-Id: txf413c02d81b448f0b7eaa-0052a72231
  Content-Type: text/plain; charset=utf-8

which basically allow the user None to Read Write there not ideal .

swift support wasn't working since last august before this commit went in so I don't think it's a big deal to fix it as security :

https://review.openstack.org/#/c/59241/
https://github.com/openstack/python-swiftclient/commit/983988093f3848671cb27b3c1956e4ca62087dff

[Chmouel Boudjnah (chmouel)]

ignore the first patch (or see it with less since it has colors/terminal chars in there)

[Steve Baker (steve-stevebaker)]

I'd like an opinion on whether this should be flagged as a security bug. I would have thought that users could only expect ACL to be applied if they specify an explicit ACL.

Can you describe how an acl of None differs from an acl of empty string?

[Chmouel Boudjnah (chmouel)]

well if there is a user or a group None you basically allow access to it, I don't think there is much changes and it maybe a rare corner case. I would not mind not setting this as security group since that feature wasn't even working for the last couple of month.

[Chmouel Boudjnah (chmouel)]

Just to make sure I am answering your question, Swift doesn't interpret None as empty string it just interpret it as the name None :)

[Thierry Carrez (ttx)]

If Havana release (or stable branch) is not vulnerable (due to the feature being broken) we should just fix this publicly. Could you confirm this is the case ?

[Chmouel Boudjnah (chmouel)]

well havanna release of heat with trunk swiftclient (as of this week) is affected.

i'm not sure if the clients follow the openstack releases, they are just independent, right?

[Thierry Carrez (ttx)]

They are independent, so I think it makes sense to issue an advisory here.

[Thomas Herve (therve)]

The patch looks fine, though maybe I would build the headers dict properly in the first place, instead of filing it with keys we then discard. You also need to fix a PEP8 issue which would make the gate sad.

[Chmouel Boudjnah (chmouel)]

@Thomas yeah i have experimented to fix property to just show an empty string when the type is String and the value is None. It seemed (from looking having to change the schema) thought that there was much more to fix in there which would be pretty intrusive for a security fix that can go to stable/

[Thierry Carrez (ttx)]

VMT team, opinion on need for OSSA on this one ?

[Jeremy Stanley (fungi)]

Yes, since users running stable/havana Heat with the next release of python-swiftclient will potentially begin granting unintended access, this warrants an advisory. The need for there to be a user specifically named "None" seems like it would make this an unlikely enough scenario to not need embargo while being fixed however. Any opinions to the contrary?

[Chmouel Boudjnah (chmouel)]

I am actually wrong, swift's keystoneauth actually only allow a tenant:user, or tenant:* or *:user form but would not authorize much on only a tenant name, I have functionally tested it :

This is a heat container with the Read ACL allowing None, which I have uploaded a README.rst file into:

stack@devstack:/opt/stack/heat$ swift stat testswift-SwiftContainerWebsite-6pscqn6gthbj
       Account: AUTH_4f904e80cacd4f258f7e06ef5bdc1da5
     Container: testswift-SwiftContainerWebsite-6pscqn6gthbj
       Objects: 0
         Bytes: 0
      Read ACL: None
     Write ACL:
       Sync To:
      Sync Key: secretg
Meta Web-Error: error.html
Meta Web-Index: index.html
   X-Timestamp: 1387054709.12011
    X-Trans-Id: txbd3d9ea6e7f44af69b38c-0052accf09
  Content-Type: text/plain; charset=utf-8
 Accept-Ranges: bytes

I have added a user and a tenant of the name of None

stack@devstack:/opt/stack/heat$ keystone user-get None
+----------+----------------------------------+
| Property | Value |
+----------+----------------------------------+
| email | |
| enabled | True |
| id | 46f4dbfbdb7b47899d29093cd31c321a |
| name | None |
| tenantId | 208116e9b9fd4f4b9bce5e0fc343c430 |
+----------+----------------------------------+
stack@devstack:/opt/stack/heat$ keystone tenant-get None
+-------------+----------------------------------+
| Property | Value |
+-------------+----------------------------------+
| description | |
| enabled | True |
| id | 208116e9b9fd4f4b9bce5e0fc343c430 |
| name | None |
+-------------+----------------------------------+

and got a valid token which I stored in the none_token variable.

stack@devstack:~/devstack$ source openrc None None
stack@devstack:~/devstack$ keystone token-get
+-----------+----------------------------------+
| Property | Value |
+-----------+----------------------------------+
| expires | 2013-12-15T21:46:50Z |
| id | 2faaccc36a0346a3a6829943fea6bcb2 |
| tenant_id | 208116e9b9fd4f4b9bce5e0fc343c430 |
| user_id | 46f4dbfbdb7b47899d29093cd31c321a |
+-----------+----------------------------------+
stack@devstack:~/devstack$ export none_token=2faaccc36a0346a3a6829943fea6bcb2

and when testing with it i get a permission denied :

stack@devstack:~/devstack$ curl -H "x-auth-token: $none_token" http://127.0.0.1:8080/v1/AUTH_4f904e80cacd4f258f7e06ef5bdc1da5/testswift-SwiftContainerWebsite-6pscqn6gthbj/README.rst;echo
<html><h1>Forbidden</h1><p>Access was denied to this resource.</p></html>

so I apologies for the noise again AFAICT this is only a 'cosmetic' bug

[Steve Baker (steve-stevebaker)]

I've set priority to low. It looks like this bug can be made public, and the fix submitted via gerrit.

[Chmouel Boudjnah (chmouel)]

Thanks, Review in https://review.openstack.org/#/c/62199

[OpenStack Infra (hudson-openstack)]

Reviewed: https://review.openstack.org/62199
Committed: https://git.openstack.org/cgit/openstack/heat/commit/?id=db64206035ac486342354825374d78be0bab5ff7
Submitter: Jenkins
Branch: master

commit db64206035ac486342354825374d78be0bab5ff7
Author: Chmouel Boudjnah <email address hidden>
Date: Sun Dec 15 18:05:03 2013 +0100

    Don't pass swift headers as None

    If we set an header as None for a container swiftclient would just pass
    it to httplib which convert it to string and set the None string into
    it. This could not be changed in swiftclient to keep backward
    compatibility lords happy so make sure that heat doesn't do that.

    Closes-Bug: 1259571
    Change-Id: Ida1d2cec87e44264f9a7d69a1e220b544b33b63c