[Libstoragemgmt-devel] [PATCH 2/4] C API: Add initiator verfication.

[Tony Asleson]

Signed-off-by: Tony Asleson <***@redhat.com>
---
c_binding/lsm_mgmt.cpp | 121 +++++++++++++++++++++++++++++++++++++++++++++++--
1 file changed, 116 insertions(+), 5 deletions(-)

diff --git a/c_binding/lsm_mgmt.cpp b/c_binding/lsm_mgmt.cpp
index 5805cf9..3fed4f9 100644
--- a/c_binding/lsm_mgmt.cpp
+++ b/c_binding/lsm_mgmt.cpp
@@ -25,6 +25,7 @@
#include <string.h>
#include <dirent.h>
#include <libxml/uri.h>
+#include <regex.h>

#include "lsm_datatypes.hpp"
#include "lsm_convert.hpp"
@@ -76,6 +77,105 @@ static int check_search_key(const char *search_key,
return 0;
}

+static int reg_ex_match(const char *pattern, const char *str)
+{
+ regex_t start_state;
+ int status = 0;
+ int rc = regcomp(&start_state, pattern, REG_EXTENDED);
+
+ if (rc) {
+ // Development only when changing regular expression
+ fprintf(stderr, "%s: bad pattern: '%s' %d\n", str, pattern, rc);
+ return -1;
+ }
+
+ status = regexec(&start_state, str, 0, NULL, 0);
+ regfree(&start_state);
+
+ return status;
+}
+
+/**
+ * Validates an iSCSI IQN
+ * @param iqn iSCSI iqn to check
+ * @return 0 on success, else error
+ */
+static int iqn_validate(const char *iqn)
+{
+ // Pattern derived from http://tools.ietf.org/html/rfc3720#page-32
+ const char *pattern =
+ "^iqn" // iqn
+ "\\."
+ "[0-9]{4}-0[1-9]|1[0-2]" // Data code yyyy-mm
+ "\\."
+ "[a-zA-Z]{2,}" // TLD
+ "\\."
+ "[a-zA-Z0-9]([a-zA-Z0-9-]{0,61})?" // Domain
+ "(\\.[a-zA-Z0-9]([a-zA-Z0-9-]{0,61})?)*" // Optional sub domain(s)
+ "(:.+)?$"; // Optional, user defined
+
+ if( iqn && strlen(iqn) ) {
+ return reg_ex_match(pattern, iqn);
+ } else {
+ return 1;
+ }
+}
+
+/**
+ * Given a WWPN validate it and then convert to internal representation.
+ * @param wwpn World wide port name to validate
+ * @return NULL if not patch, else string with common lsm format
+ */
+static char *wwpn_validate(const char *wwpn)
+{
+ size_t i = 0;
+ size_t out = 0;
+ char *rc = NULL;
+ const char *pattern = "^(0x|0X)?([0-9A-Fa-f]{2})"
+ "(([\\.\\:\\-])?[0-9A-Fa-f]{2}){7}$";
+ int bad = reg_ex_match(pattern, wwpn);
+
+ if( !bad ) {
+ rc = (char*)calloc(24,1);
+ size_t len = strlen(wwpn);
+
+ if (wwpn[1] == 'x' || wwpn[1] == 'X') {
+ i = 2;
+ }
+
+ for( ; i < len; ++i ) {
+ if( wwpn[i] != ':' && wwpn[i] != '-' && wwpn[i] != '.') {
+ rc[out++] = tolower(wwpn[i]);
+ } else {
+ rc[out++] = ':';
+ }
+ }
+ }
+ return rc;
+}
+
+static int verify_initiator_id(const char *id, lsm_access_group_init_type t,
+ Value &initiator)
+{
+ initiator = Value(id);
+
+ if( t == LSM_ACCESS_GROUP_INIT_TYPE_WWPN ) {
+ char *wwpn = wwpn_validate(id);
+ if( wwpn ) {
+ initiator = Value(wwpn);
+ free(wwpn);
+ wwpn = NULL;
+ } else {
+ return LSM_ERR_INVALID_ARGUMENT;
+ }
+ } else if( t == LSM_ACCESS_GROUP_INIT_TYPE_ISCSI_IQN ) {
+ if( iqn_validate(id) ) {
+ return LSM_ERR_INVALID_ARGUMENT;
+ }
+ }
+ return LSM_ERR_OK;
+}
+
/**
* Strings are non null with a len >= 1
*/
@@ -1177,7 +1277,7 @@ int lsm_volume_replicate(lsm_connect *c, lsm_pool *pool,
{
CONN_SETUP(c);

- if( pool && !LSM_IS_POOL(pool) || !LSM_IS_VOL(volumeSrc) ) {
+ if( (pool && !LSM_IS_POOL(pool)) || !LSM_IS_VOL(volumeSrc) ) {
return LSM_ERR_INVALID_ARGUMENT;
}

@@ -1319,8 +1419,7 @@ int lsm_iscsi_chap_auth(lsm_connect *c, const char *init_id,
{
CONN_SETUP(c);

- if( NULL == init_id || strlen(init_id) == 0 ||
- LSM_FLAG_UNUSED_CHECK(flags)) {
+ if( iqn_validate(init_id) || LSM_FLAG_UNUSED_CHECK(flags)) {
return LSM_ERR_INVALID_ARGUMENT;
}

@@ -1411,9 +1510,15 @@ int lsm_access_group_create(lsm_connect *c, const char *name,
return LSM_ERR_INVALID_ARGUMENT;
}

+ Value id;
+
+ if( LSM_ERR_OK != verify_initiator_id(init_id, init_type, id) ) {
+ return LSM_ERR_INVALID_ARGUMENT;
+ }
+
std::map<std::string, Value> p;
p["name"] = Value(name);
- p["init_id"] = Value(init_id);
+ p["init_id"] = id;
p["init_type"] = Value((int32_t)init_type);
p["system"] = system_to_value(system);
p["flags"] = Value(flags);
@@ -1466,9 +1571,15 @@ int lsm_access_group_initiator_add(lsm_connect *c,
return LSM_ERR_INVALID_ARGUMENT;
}

+ Value id;
+
+ if( LSM_ERR_OK != verify_initiator_id(init_id, init_type, id) ) {
+ return LSM_ERR_INVALID_ARGUMENT;
+ }
+
std::map<std::string, Value> p;
p["access_group"] = access_group_to_value(access_group);
- p["init_id"] = init_id;
+ p["init_id"] = id;
p["init_type"] = Value((int32_t)init_type);
p["flags"] = Value(flags);

[Tony Asleson]

The error codes existed for reason. Unfortunately some of the
checks weren't being done in code. These are mainly useful in
a badly configed or installed library setup. However, it should
be useful if any of these errors are encountered.

Signed-off-by: Tony Asleson <***@redhat.com>
---
.../include/libstoragemgmt/libstoragemgmt_error.h | 7 ++--
c_binding/lsm_datatypes.cpp | 44 ++++++++++++----------
python_binding/lsm/_common.py | 9 +++--
python_binding/lsm/_transport.py | 15 +++++++-
4 files changed, 47 insertions(+), 28 deletions(-)

diff --git a/c_binding/include/libstoragemgmt/libstoragemgmt_error.h b/c_binding/include/libstoragemgmt/libstoragemgmt_error.h
index 5ce1d2c..818eb94 100644
--- a/c_binding/include/libstoragemgmt/libstoragemgmt_error.h
+++ b/c_binding/include/libstoragemgmt/libstoragemgmt_error.h
@@ -88,10 +88,11 @@ typedef enum {
LSM_ERR_NO_SUPPORT_OFFLINE_CHANGE = 251, /**< Needs to be online to perform operation */

LSM_ERR_PLUGIN_AUTH_FAILED = 300, /**< Authorization failed */
- LSM_ERR_PLUGIN_IPC_FAIL = 301, /**< dlopen on plugin failed */
+ LSM_ERR_PLUGIN_IPC_FAIL = 301, /**< Inter-process communication between client &
+ out of process plug-in encountered connection
+ errors.**/

- LSM_ERR_PLUGIN_SOCKET_PERMISSION = 307, /**< Unable to access plugin */
- LSM_ERR_PLUGIN_REGISTRATION = 308, /**< Error during plug-in registration */
+ LSM_ERR_PLUGIN_SOCKET_PERMISSION = 307, /**< Incorrect permission on UNIX domain socket used for IPC */
LSM_ERR_PLUGIN_NOT_EXIST = 311, /**< Plug-in does not appear to exist */

LSM_ERR_NOT_ENOUGH_SPACE = 350, /**< Insufficient space */
diff --git a/c_binding/lsm_datatypes.cpp b/c_binding/lsm_datatypes.cpp
index 8e53158..939ba20 100644
--- a/c_binding/lsm_datatypes.cpp
+++ b/c_binding/lsm_datatypes.cpp
@@ -301,32 +301,36 @@ int driver_load(lsm_connect *c, const char *plugin_name, const char *password,
return LSM_ERR_NO_MEMORY;
}

- if (access(plugin_file, R_OK) == 0) {
- int ec;
- int sd = Transport::socket_get(std::string(plugin_file), ec);
-
- if( sd >= 0 ) {
- c->tp = new Ipc(sd);
- if( startup ) {
- if( connection_establish(c, password, timeout, e, flags)) {
- rc = LSM_ERR_PLUGIN_IPC_FAIL;
+ if (access(plugin_file, F_OK) != 0) {
+ rc = LSM_ERR_PLUGIN_NOT_EXIST;
+ } else {
+ if (access(plugin_file, R_OK|W_OK) == 0) {
+ int ec;
+ int sd = Transport::socket_get(std::string(plugin_file), ec);
+
+ if( sd >= 0 ) {
+ c->tp = new Ipc(sd);
+ if( startup ) {
+ if( connection_establish(c, password, timeout, e, flags)) {
+ rc = LSM_ERR_PLUGIN_IPC_FAIL;
+ }
}
+ } else {
+ *e = lsm_error_create(LSM_ERR_PLUGIN_IPC_FAIL,
+ LSM_ERR_DOMAIN_FRAME_WORK,
+ LSM_ERR_LEVEL_ERROR, "Unable to connect to plugin",
+ NULL, dlerror(), NULL, 0 );
+
+ rc = LSM_ERR_PLUGIN_IPC_FAIL;
}
} else {
- *e = lsm_error_create(LSM_ERR_PLUGIN_IPC_FAIL,
+ *e = lsm_error_create(LSM_ERR_PLUGIN_SOCKET_PERMISSION,
LSM_ERR_DOMAIN_FRAME_WORK,
- LSM_ERR_LEVEL_ERROR, "Unable to connect to plugin",
- NULL, dlerror(), NULL, 0 );
+ LSM_ERR_LEVEL_ERROR, "Unable to access plugin",
+ NULL, NULL, NULL, 0 );

- rc = LSM_ERR_PLUGIN_IPC_FAIL;
+ rc = LSM_ERR_PLUGIN_SOCKET_PERMISSION;
}
- } else {
- *e = lsm_error_create(LSM_ERR_PLUGIN_SOCKET_PERMISSION,
- LSM_ERR_DOMAIN_FRAME_WORK,
- LSM_ERR_LEVEL_ERROR, "Unable to access plugin",
- NULL, NULL, NULL, 0 );
-
- rc = LSM_ERR_PLUGIN_SOCKET_PERMISSION;
}

free(plugin_file);
diff --git a/python_binding/lsm/_common.py b/python_binding/lsm/_common.py
index d11c170..8100509 100644
--- a/python_binding/lsm/_common.py
+++ b/python_binding/lsm/_common.py
@@ -464,10 +464,13 @@ class ErrorNumber(object):
NO_SUPPORT_ONLINE_CHANGE = 250
NO_SUPPORT_OFFLINE_CHANGE = 251

- PLUGIN_AUTH_FAILED = 300
- PLUGIN_IPC_FAIL = 301
+ PLUGIN_AUTH_FAILED = 300 # Client supplied credential are incorrect
+ PLUGIN_IPC_FAIL = 301 # Inter-process communication between client &
+ # out of process plug-in encountered connection
+ # errors.

- PLUGIN_SOCKET_PERMISSION = 307
+ PLUGIN_SOCKET_PERMISSION = 307 # Incorrect permission on UNIX domain
+ # socket used for IPC
PLUGIN_REGISTRATION = 308
PLUGIN_NOT_EXIST = 311

diff --git a/python_binding/lsm/_transport.py b/python_binding/lsm/_transport.py
index 4498be2..5d6a6ec 100644
--- a/python_binding/lsm/_transport.py
+++ b/python_binding/lsm/_transport.py
@@ -18,6 +18,7 @@
import json
import socket
import string
+import os
from _common import SocketEOF as _SocketEOF
from _common import LsmError, ErrorNumber
from _data import DataDecoder as _DataDecoder, DataEncoder as _DataEncoder
@@ -98,10 +99,20 @@ class TransPort(object):
"""
try:
s = socket.socket(socket.AF_UNIX, socket.SOCK_STREAM)
- s.connect(path)
+
+ if os.path.exists(path):
+ if os.access(path, os.R_OK | os.W_OK):
+ s.connect(path)
+ else:
+ raise LsmError(ErrorNumber.PLUGIN_SOCKET_PERMISSION,
+ "Permissions are incorrect for IPC "
+ "socket file")
+ else:
+ raise LsmError(ErrorNumber.PLUGIN_NOT_EXIST,
+ "Plug-in appears to not exist")
except socket.error:
#self, code, message, data=None, *args, **kwargs
- raise LsmError(ErrorNumber.NETWORK_ERROR,
+ raise LsmError(ErrorNumber.PLUGIN_IPC_FAIL,
"Unable to connect to lsmd, daemon started?")
return s

[Tony Asleson]

I also updated the regular expressions to enforce the following:

- The month field in iqn can only be in the range 01-12
- The WWPN will fail for pattern if a ':' directly follows the 0x|0X

Signed-off-by: Tony Asleson <***@redhat.com>
---
python_binding/lsm/_data.py | 23 +++++++++--------------
1 file changed, 9 insertions(+), 14 deletions(-)

diff --git a/python_binding/lsm/_data.py b/python_binding/lsm/_data.py
index 17df3ec..23bf920 100644
--- a/python_binding/lsm/_data.py
+++ b/python_binding/lsm/_data.py
@@ -676,14 +676,13 @@ class AccessGroup(IData):
^
iqn # iqn
\. # .
- [0-9]{4}-[0-9]{2} # 1995-02
- \. # .
- [A-Za-z](?:[A-Za-z0-9\-]*[A-Za-z0-9])? # com
- \. # .
- [A-Za-z](?:[A-Za-z0-9\-]*[A-Za-z0-9])? # redhat
- : # :
- [\.A-Za-z\-0-9]+ # gris-01
- $
+ [0-9]{4}-0[1-9]|1[0-2] # Data code yyyy-mm
+ \. # .
+ [a-zA-Z]{2,} # TLD
+ \.
+ [a-zA-Z0-9]([a-zA-Z0-9-]{0,61})? # Domain
+ (\.[a-zA-Z0-9]([a-zA-Z0-9-]{0,61})?)* # Optional sub domain(s)
+ (:.+)?$ # Optional, user defined
""", re.X)

@staticmethod
@@ -708,12 +707,8 @@ class AccessGroup(IData):
return False

_regex_wwpn = re.compile(r"""
- ^
- [0x|0X]{0,1}
- (:?[0-9A-Fa-f]{2}
- [\.\-:]{0,1}){7}
- [0-9A-Fa-f]{2}
- $
+ ^(0x|0X)?([0-9A-Fa-f]{2})
+ (([\.:\-])?[0-9A-Fa-f]{2}){7}$
""", re.X)

@staticmethod

[Gris Ge]

Post by Tony Asleson
- The month field in iqn can only be in the range 01-12
- The WWPN will fail for pattern if a ':' directly follows the 0x|0X

I incorrect spelled the non-capture group as ':?', it should be '?:'.

I added non-capture group mark so that this regex could be used somewhere else
without interrupting the re.match.group().

This is the updated version:

_wwpn_regex = re.compile(r"""
^
(?:0x|0X)? # 0x or 0X, optional
(?:[0-9A-Fa-f]{2} # First hex group.
[\.:\-]?) # First separator, optional
{7} # Repeat hex group separator 7 times.
[0-9A-Fa-f]{2} # Last hex group
$
""", re.X)

In RFC 3722, iSCSI iqn only allow [a-z\-\.\:].
The upper-case should be converted by user application(like lsmcli).

This is the updated version for iscsi iqn:

_regex_iscsi_iqn = re.compile(r"""
^
iqn # used to distinguish from "eui."
\.
[0-9]{4}-(?:0[1-9]|1[0-2]) # Data code yyyy-mm
\.
[a-z0-9\-]{2,} # Top level of domain
# '-' is for internationalized
# domain, like .xn--3ds443g
\.
[a-z0-9][a-z0-9\-]{0,62} # Second level of domain
# Domain length 63 is defined in
# RFC 2181 section 11

(?:\.[a-z0-9][a-z0-9\-]{0,63})* # Lower levels of domain, optional

(?:\:[a-z0-9\-\.]+) # Optional, user defined
$
""", re.X)


I have attached two scripts to test these two regexes.

[1] https://docs.python.org/2/howto/regex.html#non-capturing-and-named-groups

[Tony Asleson]

Post by Gris Ge

Post by Tony Asleson
- The month field in iqn can only be in the range 01-12
- The WWPN will fail for pattern if a ':' directly follows the 0x|0X

I incorrect spelled the non-capture group as ':?', it should be '?:'.
I added non-capture group mark so that this regex could be used somewhere else
without interrupting the re.match.group().

I was trying to make the C and Python versions the same. C doesn't
support this, thus the reason for it being omitted. We aren't using the
group functionality AFAIK so we should be able to leave out.

Post by Gris Ge
In RFC 3722, iSCSI iqn only allow [a-z\-\.\:].
The upper-case should be converted by user application(like lsmcli).
_regex_iscsi_iqn = re.compile(r"""
^
iqn # used to distinguish from "eui."
\.
[0-9]{4}-(?:0[1-9]|1[0-2]) # Data code yyyy-mm
\.
[a-z0-9\-]{2,} # Top level of domain
# '-' is for internationalized
# domain, like .xn--3ds443g

Don't believe you can have '-' at end so possibly

[a-z0-9][a-z0-9\-]*[a-z0-9]

Post by Gris Ge
\.
[a-z0-9][a-z0-9\-]{0,62} # Second level of domain
# Domain length 63 is defined in
# RFC 2181 section 11

Same as above, no ending '-'

[a-z0-9][a-z0-9\-]{0,61}[a-z0-9]

Post by Gris Ge
(?:\.[a-z0-9][a-z0-9\-]{0,63})* # Lower levels of domain, optional

Same as above, no ending '-'

I don't think we can actually construct a regular expression that
validates the domain correctly, eg. you can have (2) hyphens in a row,
but not in all positions and you can't have (3) or more consecutively.
In addition the total length of domain and TLD cannot exceed 63 characters.

My initial thought when you submitted the original patch was to reject
it because I can see some different possibilities here:

1. We adhere to the specification perfectly, but the vendor doesn't so
we pass the string to the array and array rejects
2. We have a bug in our regular expression and we allow something that
shouldn't be and the array rejects it

After looking at this some more I'm starting to think we should drop the
iqn validation, just let the array do it.

All of this reminds me of:

http://blog.codinghorror.com/regular-expressions-now-you-have-two-problems/

Others thoughts?

Regards,
Tony

[Andy Grover]

Post by Tony Asleson
1. We adhere to the specification perfectly, but the vendor doesn't so
we pass the string to the array and array rejects
2. We have a bug in our regular expression and we allow something that
shouldn't be and the array rejects it
After looking at this some more I'm starting to think we should drop the
iqn validation, just let the array do it.
http://blog.codinghorror.com/regular-expressions-now-you-have-two-problems/
Others thoughts?

I'm in favor of no (or very light) checking of iqns. (Or "iSCSI names",
of which IQN is the most popular subtype definition.) They're somewhat
loosely defined, and not everyone follows the format, so you could emit
a warning but aborting the command might be a little severe. As long as
they're uniquely identifiable that might be all that matters.

For WWPNs, they are different because you can have multiple formats that
are actually the same value, once you normalize all the "0x"s and
colons, dashes, or dots that may be separating the octets. So those we
probably want to continue to normalize and check, to avoid duplicates.

my 2c -- Regards -- Andy

[Gris Ge]

Post by Andy Grover

Post by Tony Asleson
1. We adhere to the specification perfectly, but the vendor doesn't so
we pass the string to the array and array rejects
2. We have a bug in our regular expression and we allow something that
shouldn't be and the array rejects it
After looking at this some more I'm starting to think we should drop the
iqn validation, just let the array do it.
http://blog.codinghorror.com/regular-expressions-now-you-have-two-problems/
Others thoughts?

I'm in favor of no (or very light) checking of iqns. (Or "iSCSI
names", of which IQN is the most popular subtype definition.)
They're somewhat loosely defined, and not everyone follows the
format, so you could emit a warning but aborting the command might
be a little severe. As long as they're uniquely identifiable that
might be all that matters.

How about we just check the leading 'iqn.' and leave the rest check to array?