[PATCH master 0/4] Implement separate access option for queries over RAPI

[Michael Hanselmann]

Issue 301 requests a separate option for granting query permissions to RAPI
users without giving them write permissions.

Michael Hanselmann (4):
Warn on invalid lines in HTTP user files
rapi: Add new user option for querying
Update ganeti-rapi man page for new user option
Add unit test for RAPI handler access definitions

NEWS | 3 +
doc/rapi.rst | 50 +++++++++++--
lib/http/auth.py | 4 +
lib/rapi/__init__.py | 6 ++
lib/rapi/rlib2.py | 5 +-
man/ganeti-rapi.rst | 21 +-----
test/ganeti.rapi.rlib2_unittest.py | 24 ++++++
test/ganeti.server.rapi_unittest.py | 133 +++++++++++++++++++----------------
8 files changed, 158 insertions(+), 88 deletions(-)

--
1.7.7.3

[Michael Hanselmann]

Instead of duplicating what is already described in the full
documentation, a reference is added instead. Man pages can't use
constants or assertions, therefore it is easier for them to get out of
sync with the code.

Signed-off-by: Michael Hanselmann <han...@google.com>
---
man/ganeti-rapi.rst | 21 +++------------------
1 files changed, 3 insertions(+), 18 deletions(-)

diff --git a/man/ganeti-rapi.rst b/man/ganeti-rapi.rst
index ed96997..ad78e85 100644
--- a/man/ganeti-rapi.rst
+++ b/man/ganeti-rapi.rst
@@ -34,28 +34,13 @@ in the same format as for the node and master daemon.
ACCESS CONTROLS
---------------

-All query operations are allowed without authentication. Only the
+Most query operations are allowed without authentication. Only the
modification operations require authentication, in the form of basic
authentication.

The users and their rights are defined in the
-``@LOCALSTATEDIR@/lib/ganeti/rapi/users`` file. The users
-should be listed one per line, in the following format::
-
- username password options
-
-Currently the *options* field should equal the string ``write`` in
-order to actually give write permission for the given users. Example::
-
- rclient secret write
- guest testpw
-
-The first user (*rclient*) has read-write rights, whereas the second
-user (*guest*) only has read (query) rights, and as such is no
-different than not using authentication at all.
-
-More details (including on how to use hashed passwords) can be found
-in the Ganeti documentation.
+``@LOCALSTATEDIR@/lib/ganeti/rapi/users`` file. Format of this file is
+described in the Ganeti documentation (``rapi.html``).

.. vim: set textwidth=72 :
.. Local Variables:
--
1.7.7.3

[Michael Hanselmann]

Without this change, invalid lines or values would be silently ignored.
---
lib/http/auth.py | 4 ++++
1 files changed, 4 insertions(+), 0 deletions(-)

diff --git a/lib/http/auth.py b/lib/http/auth.py
index 1e33f2c..5dec95b 100644
--- a/lib/http/auth.py
+++ b/lib/http/auth.py
@@ -310,6 +310,8 @@ def ParsePasswordFile(contents):
parts = line.split(None, 2)
if len(parts) < 2:
# Invalid line
+ # TODO: Return line number from FilterEmptyLinesAndComments
+ logging.warning("Ignoring non-comment line with less than two fields")
continue

name = parts[0]
@@ -320,6 +322,8 @@ def ParsePasswordFile(contents):
if len(parts) >= 3:
for part in parts[2].split(","):
options.append(part.strip())
+ else:
+ logging.warning("Ignoring values for user '%s': %s", name, parts[3:])

users[name] = PasswordFileUser(name, password, options)

--
1.7.7.3

[Michael Hanselmann]

This was requested in issue 301. Before this patch, requests to
“/2/query/*” and “/2/instances/*/console” would require authentication
with a user with write access. Since that is not strictly necessary, a
new user option named “query” is added.

Console information can also be retrieved as a normal query, therefore
the change applies there too.

This was the first user option to be added after “write”, therefore
quite a few changes were necessary. Documentation, including NEWS, is
updated as well.

Signed-off-by: Michael Hanselmann <han...@google.com>
---

NEWS | 3 +
doc/rapi.rst | 50 +++++++++++--

lib/rapi/__init__.py | 6 ++
lib/rapi/rlib2.py | 5 +-

test/ganeti.server.rapi_unittest.py | 133 +++++++++++++++++++----------------
5 files changed, 127 insertions(+), 70 deletions(-)

diff --git a/NEWS b/NEWS
index 45f081c..fd99f90 100644
--- a/NEWS
+++ b/NEWS
@@ -29,6 +29,9 @@ Version 2.7.0 beta1
``gnt-node add`` now invokes a new tool on the destination node, named
``prepare-node-join``, to configure the SSH daemon. Paramiko is no
longer necessary to configure nodes' SSH daemons via ``gnt-node add``.
+- A new user option, :pyeval:`rapi.RAPI_ACCESS_QUERY`, has been added
+ for RAPI users. It allows granting query permissions to a specific
+ user without giving :pyeval:`rapi.RAPI_ACCESS_WRITE` permissions.


Version 2.6.1
diff --git a/doc/rapi.rst b/doc/rapi.rst
index c46051a..e9bc17d 100644
--- a/doc/rapi.rst
+++ b/doc/rapi.rst
@@ -24,12 +24,24 @@ Users and passwords
``/var/lib/ganeti/rapi/users``) on startup. Changes to the file will be
read automatically.

-Each line consists of two or three fields separated by whitespace. The
-first two fields are for username and password. The third field is
-optional and can be used to specify per-user options. Currently,
-``write`` is the only option supported and enables the user to execute
-operations modifying the cluster. Lines starting with the hash sign
-(``#``) are treated as comments.
+Lines starting with the hash sign (``#``) are treated as comments. Each
+line consists of two or three fields separated by whitespace. The first
+two fields are for username and password. The third field is optional
+and can be used to specify per-user options (separated by comma without
+spaces). Available options:
+
+.. pyassert::
+
+ rapi.RAPI_ACCESS_ALL == set([
+ rapi.RAPI_ACCESS_WRITE,
+ rapi.RAPI_ACCESS_QUERY,
+ ])
+
+:pyeval:`rapi.RAPI_ACCESS_WRITE`
+ Enables the user to execute operations modifying the cluster. Implies
+ :pyeval:`rapi.RAPI_ACCESS_QUERY` access.
+:pyeval:`rapi.RAPI_ACCESS_QUERY`
+ Allow access to operations querying for information.

Passwords can either be written in clear text or as a hash. Clear text
passwords may not start with an opening brace (``{``) or they must be
@@ -51,6 +63,12 @@ Example::
# Hashed password for Jessica
jessica {HA1}7046452df2cbb530877058712cf17bd4 write

+ # Monitoring can query for values
+ monitoring {HA1}ec018ffe72b8e75bb4d508ed5b6d079c query
+
+ # A user who can query and write
+ superuser {HA1}ec018ffe72b8e75bb4d508ed5b6d079c query,write
+

.. [#pwhash] Using the MD5 hash of username, realm and password is
described in :rfc:`2617` ("HTTP Authentication"), sections 3.2.2.2
@@ -1121,7 +1139,15 @@ Job result:

Request information for connecting to instance's console.

-Supports the following commands: ``GET``.
+.. pyassert::
+
+ not (hasattr(rlib2.R_2_instances_name_console, "PUT") or
+ hasattr(rlib2.R_2_instances_name_console, "POST") or
+ hasattr(rlib2.R_2_instances_name_console, "DELETE"))
+
+Supports the following commands: ``GET``. Requires authentication with
+one of the following options:
+:pyeval:`utils.CommaJoin(rlib2.R_2_instances_name_console.GET_ACCESS)`.

``GET``
~~~~~~~
@@ -1640,7 +1666,15 @@ pages and using ``/2/query/[resource]/fields``. The resource is one of
:pyeval:`utils.CommaJoin(constants.QR_VIA_RAPI)`. See the :doc:`query2
design document <design-query2>` for more details.

-Supports the following commands: ``GET``, ``PUT``.
+.. pyassert::
+
+ (rlib2.R_2_query.GET_ACCESS == rlib2.R_2_query.PUT_ACCESS and
+ not (hasattr(rlib2.R_2_query, "POST") or
+ hasattr(rlib2.R_2_query, "DELETE")))
+
+Supports the following commands: ``GET``, ``PUT``. Requires
+authentication with one of the following options:
+:pyeval:`utils.CommaJoin(rlib2.R_2_query.GET_ACCESS)`.

``GET``
~~~~~~~
diff --git a/lib/rapi/__init__.py b/lib/rapi/__init__.py
index 28d6ead..49a97a8 100644
--- a/lib/rapi/__init__.py
+++ b/lib/rapi/__init__.py
@@ -21,3 +21,9 @@
"""Ganeti RAPI module"""

RAPI_ACCESS_WRITE = "write"
+RAPI_ACCESS_QUERY = "query"
+
+RAPI_ACCESS_ALL = frozenset([
+ RAPI_ACCESS_WRITE,
+ RAPI_ACCESS_QUERY,
+ ])
diff --git a/lib/rapi/rlib2.py b/lib/rapi/rlib2.py
index d31a5b4..e47cf24 100644
--- a/lib/rapi/rlib2.py
+++ b/lib/rapi/rlib2.py
@@ -1226,7 +1226,7 @@ class R_2_instances_name_console(baserlib.ResourceBase):
"""/2/instances/[instance_name]/console resource.

"""
- GET_ACCESS = [rapi.RAPI_ACCESS_WRITE]
+ GET_ACCESS = [rapi.RAPI_ACCESS_WRITE, rapi.RAPI_ACCESS_QUERY]
GET_OPCODE = opcodes.OpInstanceConsole

def GET(self):
@@ -1278,7 +1278,8 @@ class R_2_query(baserlib.ResourceBase):

"""
# Results might contain sensitive information
- GET_ACCESS = [rapi.RAPI_ACCESS_WRITE]
+ GET_ACCESS = [rapi.RAPI_ACCESS_WRITE, rapi.RAPI_ACCESS_QUERY]
+ PUT_ACCESS = GET_ACCESS
GET_OPCODE = opcodes.OpQuery
PUT_OPCODE = opcodes.OpQuery

diff --git a/test/ganeti.server.rapi_unittest.py b/test/ganeti.server.rapi_unittest.py
index 618a44e..cca3a55 100755
--- a/test/ganeti.server.rapi_unittest.py
+++ b/test/ganeti.server.rapi_unittest.py
@@ -168,67 +168,80 @@ class TestRemoteApiHandler(unittest.TestCase):
else:
return None

- def _LookupUserWithWrite(name):
- if name == username:
- return http.auth.PasswordFileUser(name, password, [
- rapi.RAPI_ACCESS_WRITE,
- ])
- else:
- return None
-
- for qr in constants.QR_VIA_RAPI:
- # The /2/query resource has somewhat special rules for authentication as
- # it can be used to retrieve critical information
- path = "/2/query/%s" % qr
-
- for method in rapi.baserlib._SUPPORTED_METHODS:
- # No authorization
- (code, _, _) = self._Test(method, path, "", "")
-
- if method in (http.HTTP_DELETE, http.HTTP_POST):
- self.assertEqual(code, http.HttpNotImplemented.code)
- continue
-
- self.assertEqual(code, http.HttpUnauthorized.code)
-
- # Incorrect user
- (code, _, _) = self._Test(method, path, header_fn(True), "",
- user_fn=self._LookupWrongUser)
- self.assertEqual(code, http.HttpUnauthorized.code)
-
- # User has no write access, but the password is correct
- (code, _, _) = self._Test(method, path, header_fn(True), "",
- user_fn=_LookupUserNoWrite)
- self.assertEqual(code, http.HttpForbidden.code)
-
- # Wrong password and no write access
- (code, _, _) = self._Test(method, path, header_fn(False), "",
- user_fn=_LookupUserNoWrite)
- self.assertEqual(code, http.HttpUnauthorized.code)
-
- # Wrong password with write access
- (code, _, _) = self._Test(method, path, header_fn(False), "",
- user_fn=_LookupUserWithWrite)
- self.assertEqual(code, http.HttpUnauthorized.code)
-
- # Prepare request information
- if method == http.HTTP_PUT:
- reqpath = path
- body = serializer.DumpJson({
- "fields": ["name"],
- })
- elif method == http.HTTP_GET:
- reqpath = "%s?fields=name" % path
- body = ""
+ for access in [rapi.RAPI_ACCESS_WRITE, rapi.RAPI_ACCESS_QUERY]:
+ def _LookupUserWithWrite(name):
+ if name == username:
+ return http.auth.PasswordFileUser(name, password, [
+ access,
+ ])
else:
- self.fail("Unknown method '%s'" % method)
-
- # User has write access, password is correct
- (code, _, data) = self._Test(method, reqpath, header_fn(True), body,
- user_fn=_LookupUserWithWrite,
- luxi_client=_FakeLuxiClientForQuery)
- self.assertEqual(code, http.HTTP_OK)
- self.assertTrue(objects.QueryResponse.FromDict(data))
+ return None
+
+ for qr in constants.QR_VIA_RAPI:
+ # The /2/query resource has somewhat special rules for authentication as
+ # it can be used to retrieve critical information
+ path = "/2/query/%s" % qr
+
+ for method in rapi.baserlib._SUPPORTED_METHODS:
+ # No authorization
+ (code, _, _) = self._Test(method, path, "", "")
+
+ if method in (http.HTTP_DELETE, http.HTTP_POST):
+ self.assertEqual(code, http.HttpNotImplemented.code)
+ continue
+
+ self.assertEqual(code, http.HttpUnauthorized.code)
+
+ # Incorrect user
+ (code, _, _) = self._Test(method, path, header_fn(True), "",
+ user_fn=self._LookupWrongUser)
+ self.assertEqual(code, http.HttpUnauthorized.code)
+
+ # User has no write access, but the password is correct
+ (code, _, _) = self._Test(method, path, header_fn(True), "",
+ user_fn=_LookupUserNoWrite)
+ self.assertEqual(code, http.HttpForbidden.code)
+
+ # Wrong password and no write access
+ (code, _, _) = self._Test(method, path, header_fn(False), "",
+ user_fn=_LookupUserNoWrite)
+ self.assertEqual(code, http.HttpUnauthorized.code)
+
+ # Wrong password with write access
+ (code, _, _) = self._Test(method, path, header_fn(False), "",
+ user_fn=_LookupUserWithWrite)
+ self.assertEqual(code, http.HttpUnauthorized.code)
+
+ # Prepare request information
+ if method == http.HTTP_PUT:
+ reqpath = path
+ body = serializer.DumpJson({
+ "fields": ["name"],
+ })
+ elif method == http.HTTP_GET:
+ reqpath = "%s?fields=name" % path
+ body = ""
+ else:
+ self.fail("Unknown method '%s'" % method)
+
+ # User has write access, password is correct
+ (code, _, data) = self._Test(method, reqpath, header_fn(True), body,
+ user_fn=_LookupUserWithWrite,
+ luxi_client=_FakeLuxiClientForQuery)
+ self.assertEqual(code, http.HTTP_OK)
+ self.assertTrue(objects.QueryResponse.FromDict(data))
+
+ def testConsole(self):
+ path = "/2/instances/inst1.example.com/console"
+
+ for method in rapi.baserlib._SUPPORTED_METHODS:
+ # No authorization
+ (code, _, _) = self._Test(method, path, "", "")
+
+ if method == http.HTTP_GET:
+ self.assertEqual(code, http.HttpUnauthorized.code)
+ else:
+ self.assertEqual(code, http.HttpNotImplemented.code)


class _FakeLuxiClientForQuery:
--
1.7.7.3

[Michael Hanselmann]

- Ensure query-related resources have the same access permissions
(specifically “/2/query/*” and “/2/*/console”)
- Check access permission consistency (write implies query)

Signed-off-by: Michael Hanselmann <han...@google.com>
---

test/ganeti.rapi.rlib2_unittest.py | 24 ++++++++++++++++++++++++
1 files changed, 24 insertions(+), 0 deletions(-)

diff --git a/test/ganeti.rapi.rlib2_unittest.py b/test/ganeti.rapi.rlib2_unittest.py
index be64b4a..d2413b2 100755
--- a/test/ganeti.rapi.rlib2_unittest.py
+++ b/test/ganeti.rapi.rlib2_unittest.py
@@ -35,8 +35,11 @@ from ganeti import http
from ganeti import query
from ganeti import luxi
from ganeti import errors
+from ganeti import rapi

from ganeti.rapi import rlib2
+from ganeti.rapi import baserlib
+from ganeti.rapi import connector

import testutils

@@ -1753,5 +1756,26 @@ class TestInstancesMultiAlloc(unittest.TestCase):
for inst in body["instances"]]))


+class TestPermissions(unittest.TestCase):
+ def testEquality(self):
+ self.assertEqual(rlib2.R_2_query.GET_ACCESS, rlib2.R_2_query.PUT_ACCESS)
+ self.assertEqual(rlib2.R_2_query.GET_ACCESS,
+ rlib2.R_2_instances_name_console.GET_ACCESS)
+
+ def testMethodAccess(self):
+ for handler in connector.CONNECTOR.values():
+ for method in baserlib._SUPPORTED_METHODS:
+ access = getattr(handler, "%s_ACCESS" % method)
+ self.assertFalse(set(access) - rapi.RAPI_ACCESS_ALL,
+ msg=("Handler '%s' uses unknown access options for"
+ " method %s" % (handler, method)))
+ self.assertTrue(rapi.RAPI_ACCESS_QUERY not in access or
+ rapi.RAPI_ACCESS_WRITE in access,
+ msg=("Handler '%s' gives query, but not write access"
+ " for method %s (the latter includes query and"
+ " should therefore be given as well)" %
+ (handler, method)))
+
+
if __name__ == "__main__":
testutils.GanetiTestProgram()
--
1.7.7.3

[Iustin Pop]

On Wed, Nov 14, 2012 at 02:59:17AM +0100, Michael Hanselmann wrote:
> This was requested in issue 301. Before this patch, requests to
> “/2/query/*” and “/2/instances/*/console” would require authentication
> with a user with write access. Since that is not strictly necessary, a
> new user option named “query” is added.
>
> Console information can also be retrieved as a normal query, therefore
> the change applies there too.
>
> This was the first user option to be added after “write”, therefore
> quite a few changes were necessary. Documentation, including NEWS, is
> updated as well.

write/query is confusing. Please use standard write/read names.

iustin

[Guido Trotter]

LGTM

Thanks,

Guido

--
Guido Trotter
SRE - Corp Computing Services (aka Horsepower)
Google Germany

[Guido Trotter]

LGTM

Thanks,

Guido


On Wed, Nov 14, 2012 at 2:59 AM, Michael Hanselmann <han...@google.com> wrote:

[Guido Trotter]

LGTM, although it maybe would have been better to make it
non-mistakable rather than having to check.

Thanks,

Guido


On Wed, Nov 14, 2012 at 2:59 AM, Michael Hanselmann <han...@google.com> wrote:

[Michael Hanselmann]

This was requested in issue 301. Before this patch, requests to
“/2/query/*” and “/2/instances/*/console” would require authentication
with a user with write access. Since that is not strictly necessary, a

new user option named “read” is added.

Console information can also be retrieved as a normal query, therefore
the change applies there too.

This was the first user option to be added after “write”, therefore
quite a few changes were necessary. Documentation, including NEWS, is
updated as well.

Signed-off-by: Michael Hanselmann <han...@google.com>
---

NEWS | 4 +

doc/rapi.rst | 50 +++++++++++--
lib/rapi/__init__.py | 6 ++
lib/rapi/rlib2.py | 5 +-
test/ganeti.server.rapi_unittest.py | 133 +++++++++++++++++++----------------

5 files changed, 128 insertions(+), 70 deletions(-)

diff --git a/NEWS b/NEWS
index 45f081c..0e4fa66 100644
--- a/NEWS
+++ b/NEWS
@@ -29,6 +29,10 @@ Version 2.7.0 beta1

``gnt-node add`` now invokes a new tool on the destination node, named
``prepare-node-join``, to configure the SSH daemon. Paramiko is no
longer necessary to configure nodes' SSH daemons via ``gnt-node add``.

+- A new user option, :pyeval:`rapi.RAPI_ACCESS_READ`, has been added
+ for RAPI users. It allows granting permissions to query for
+ information to a specific user without giving
+ :pyeval:`rapi.RAPI_ACCESS_WRITE` permissions.

Version 2.6.1
diff --git a/doc/rapi.rst b/doc/rapi.rst

index c46051a..179edfd 100644

--- a/doc/rapi.rst
+++ b/doc/rapi.rst
@@ -24,12 +24,24 @@ Users and passwords
``/var/lib/ganeti/rapi/users``) on startup. Changes to the file will be
read automatically.

-Each line consists of two or three fields separated by whitespace. The
-first two fields are for username and password. The third field is
-optional and can be used to specify per-user options. Currently,
-``write`` is the only option supported and enables the user to execute
-operations modifying the cluster. Lines starting with the hash sign
-(``#``) are treated as comments.
+Lines starting with the hash sign (``#``) are treated as comments. Each
+line consists of two or three fields separated by whitespace. The first
+two fields are for username and password. The third field is optional
+and can be used to specify per-user options (separated by comma without
+spaces). Available options:
+
+.. pyassert::
+
+ rapi.RAPI_ACCESS_ALL == set([
+ rapi.RAPI_ACCESS_WRITE,

+ rapi.RAPI_ACCESS_READ,

+ ])
+
+:pyeval:`rapi.RAPI_ACCESS_WRITE`
+ Enables the user to execute operations modifying the cluster. Implies

+ :pyeval:`rapi.RAPI_ACCESS_READ` access.
+:pyeval:`rapi.RAPI_ACCESS_READ`

index 28d6ead..be08b2a 100644

--- a/lib/rapi/__init__.py
+++ b/lib/rapi/__init__.py
@@ -21,3 +21,9 @@
"""Ganeti RAPI module"""

RAPI_ACCESS_WRITE = "write"

+RAPI_ACCESS_READ = "read"

+
+RAPI_ACCESS_ALL = frozenset([
+ RAPI_ACCESS_WRITE,

+ RAPI_ACCESS_READ,

+ ])
diff --git a/lib/rapi/rlib2.py b/lib/rapi/rlib2.py

index d31a5b4..3a042a8 100644

--- a/lib/rapi/rlib2.py
+++ b/lib/rapi/rlib2.py
@@ -1226,7 +1226,7 @@ class R_2_instances_name_console(baserlib.ResourceBase):
"""/2/instances/[instance_name]/console resource.

"""
- GET_ACCESS = [rapi.RAPI_ACCESS_WRITE]

+ GET_ACCESS = [rapi.RAPI_ACCESS_WRITE, rapi.RAPI_ACCESS_READ]

GET_OPCODE = opcodes.OpInstanceConsole

def GET(self):
@@ -1278,7 +1278,8 @@ class R_2_query(baserlib.ResourceBase):

"""
# Results might contain sensitive information
- GET_ACCESS = [rapi.RAPI_ACCESS_WRITE]

+ GET_ACCESS = [rapi.RAPI_ACCESS_WRITE, rapi.RAPI_ACCESS_READ]

+ PUT_ACCESS = GET_ACCESS
GET_OPCODE = opcodes.OpQuery
PUT_OPCODE = opcodes.OpQuery

diff --git a/test/ganeti.server.rapi_unittest.py b/test/ganeti.server.rapi_unittest.py

index 618a44e..735c300 100755

+ for access in [rapi.RAPI_ACCESS_WRITE, rapi.RAPI_ACCESS_READ]:

[Guido Trotter]

LGTM

Thanks,

Guido