Implementation of SSL in libqnio #12

ketonne · 2017-01-29T03:34:57Z

Client will use key/cert under /var/lib/libqnio as .key and
<instanceid.cert>
Server will use key/cert under same directory as server.key and server.cert
Server may choose to validate client cert.
Certificates will be provisioned by an orchestration layer before VM
is provisioned
libqnio will expect the certificates to be available before it is initialized
otherwise will default to unsecure mode.

Client will pass instance id (VM id) for authorization on server.

Client will use key/cert under /var/lib/libqnio as <instanceid>.key and <instanceid.cert> Server will use key/cert under same directory as server.key and server.cert Server may choose to validate client cert. Certificates will be provisioned by an orchestration layer before VM is provisioned libqnio will expect the certificates to be available before it is initialized otherwise will default to unsecure mode. Client will pass instance id (VM id) for authorization on server.

stefanha · 2017-02-02T09:19:58Z

src/lib/qnio/io_ssl.c

+readv_ssl(struct endpoint *endpoint, struct iovec *vec, int count) 
+{
+    assert(endpoint->ssl != NULL);
+    return (SSL_read(endpoint->ssl, vec[0].iov_base, vec[0].iov_len));


This is a bug waiting to happen. The function doesn't look at the count argument and assumes the iovec has exactly one element. I suggest dropping readv_ssl() and writev_ssl().

The core multiplexer in libqnio calls the readv/writev for io_socket for better performance. To keep the interface consistent for the multiplexer I am using the same call for io_ssl. However since there is no native readv/writev for SSL, this implementation defaults to regular SSL read/write. The iovec adjustments for partial reads/writes are handled in nio_core.c (write_to_network and read_from_network) but the interface remains consistent.

I see. The semantics of this function do not match readv()/writev(), so it would be clearer to choose a different name but it's a very minor issue.

stefanha · 2017-02-02T09:23:41Z

src/include/utils.h

@@ -16,5 +16,8 @@ int make_socket_non_blocking(int sfd);
 char *safe_strncpy(char *dest, const char *src, size_t n);
 int compare_key(const void *x, const void *y);
 int compare_int(const void *x, const void *y);
+SSL_CTX *init_server_ssl_ctx(); 


init_server_ssl_ctx() is not the same as init_server_ssl_ctx(void) in C. The compiler will allow callers to pass any arguments and there is no type checking for init_server_ssl_ctx(). Use init_server_ssl_ctx(void) for void functions in C. (C++ is different.)

Ok. Will fix this.

Fix checked into branch.

stefanha · 2017-02-02T09:35:43Z

src/lib/qnio/utils.c

+    if (SSL_CTX_use_certificate_file(ctx, clientcert, SSL_FILETYPE_PEM) < 0) 
+    {
+        nioDbg("Unable to use client certificate");
+        return NULL;


ctx is leaked here and below.

Ok will fix.

Fix checked into branch.

stefanha · 2017-02-02T09:39:49Z

src/lib/qnio/nio_server.c

+                    {
+                        ssl = SSL_new(cmn_ctx->ssl_ctx);
+                        SSL_set_fd(ssl, infd);
+                        ret = SSL_accept(ssl);


This is blocking. A client can DoS the server by never completing the handshake. No other clients can connect during this time. This needs to be non-blocking.

Ok. I will take care of this in the vxhs server implementation. The server implementation in libqnio is a sample meant only to validate the client function independently. It need not have all the features that the vxhs IO head would have. VxHS does not use the server implementation from libqnio.

Correctness isn't a "feature". If you don't want to write asynchronous code then don't use an event loop architecture. Even if Veritas doesn't use this library for the server side, you are providing this reference implementation and it should do things properly. It's bad for VxHS if people get the impression that corners were cut and things weren't done properly.

Ok. We will be testing with the asynchronous accept with VxHS server. I will implement this in libqnio as well.

Implemented non-blocking accept. Please check the latest patch.

stefanha · 2017-02-02T09:42:08Z

src/test/server.c

@@ -125,6 +137,17 @@ void *pdispatch(void *data)
        printf("In server callback for msg #%ld\n", msg->hinfo.cookie);
    }

+    if (!authorize(msg->hinfo.target, msg->hinfo.instance))


What is the point of the instance field? The server should look at the client cert associated with this connection, not the (untrusted) instance ID passed by the client.

The instance field is to authorize the IO request. We had talked about a compromised qemu process trying to send an IO on a vdisk that it does not own. The authorize function will use information populated during the provisioning of the vdisk to check if the instance is allowed to do IO on this vdisk. The true implementation of authorize in the vxhs server will take care o this. This will also ensure that remote IOs for resiliency are authorized in a similar manner.

But why is a field necessary for this? The server already saw the client's X.509 certificate so the server can keep a per-connection ID field. The client doesn't need to send an instance ID with every request since the server knows which client it is talking to.

I implemented this in case of a service failure event, libqnio will talk to a directly to a remote VxHS server which should again authorize the client based on its X.509 cert. But come to think of it, before such an event a client should have successfully authorized itself atleast once with the local VxHS server. So maybe you are right, I don't need this extra authorization.

Have removed the instance id from message header.

berrange

IMHO libqnio should not be doing any SSL/TLS work itself whatsoever. This should all be able to be delegated to QEMU, so that it can use its normal TLS management APIs ensuring consistent behaviour and configuration with other parts of QEMU using TLS. The use of OpenSSL may also make this license incompatible with QEMU, since there are people who consider the OpenSSL License to be incompatible with the GPL license.

To achieve separation of responsibilities, libqnio would have to not do socket I/O / connections directly itself. INstead it would provide a way for an app to register I/O callbacks. eg QEMU would register two functions that libqnio could then use for read + write on the socket. These callbacks would automatically apply SSL as configured by QEMU.

stefanha · 2017-02-04T07:38:59Z

@berrange Pushing TLS to QEMU isn't possible since this is a stand-alone library similar to libglusterfs or librados. It doesn't make sense to require every library user to reimplement TLS.

codyprime

For the libqnio namechange, You are also going to want to change the following, I assume:

include/qnio.h:31:#define SECURE_IMPL "/var/lib/libqnio/secure"
include/qnio.h:32:#define SERVER_KEY "/var/lib/libqnio/server.key"
include/qnio.h:33:#define SERVER_CERT "/var/lib/libqnio/server.cert"
include/qnio.h:34:#define CLIENT_KEYSTORE "/var/lib/libqnio/"
lib/qnio/iioapi.c:20:#define VDISK_TARGET_DIR "/var/lib/libqnio/vdisk"

codyprime · 2017-02-07T20:38:36Z

src/lib/qnio/nio_server.c

+                        ret = SSL_accept(ssl);
+                        if (ret <= 0) {
+                            nioDbg("ssl accept failed %d", SSL_get_error(ssl, ret));
+                            return -1;


'ssl' is leaked here.

codyprime · 2017-02-07T20:40:10Z

src/lib/qnio/nio_client.c

+        ssl = SSL_new(cmn_ctx->ssl_ctx);
+        SSL_set_fd(ssl, sock);
+        if (SSL_connect(ssl) == -1) {
+            nioDbg("ssl connect failed");


ssl is leaked here.

It looks like is also leaked down in the out: error handling, and orphaned with the 'free(c)':

out:
if(sock != -1) {
close(sock);
}
if(infos) {
freeaddrinfo(infos);
}
if(c) {
free(c);
}

Fixed. Please check latest patch.

(1) Open file in correct mode. (2) Close the file when done so as to reset the file pointer back to zero offset.

2. Removed instance id from message header 3. Made SSL accept non-blocking 4. Addressed a couple of leaks

… into securify

Fixes for some issues that were breaking qemu-io test.

… into securify

Fix for the following stacktrace: (gdb) bt 0 0x00007f526163cdee in __strncpy_ssse3 () from /lib64/libc.so.6 1 0x00007f52643c4ba9 in iio_open (uri=0x7f5266432300 "of://127.0.0.1:9999", devid=0x7f5266431a80 "/t.raw", flags=0) at lib/qnio/iioapi.c:487 2 0x00007f5264d17f3e in vxhs_open (bs=<value optimized out>, options=<value optimized out>, bdrv_flags=<value optimized out>, errp=0x7fffbcd44308) at block/vxhs.c:308 3 0x00007f5264cbc328 in bdrv_open_common (filename=<value optimized out>, reference=<value optimized out>, options=0x7f5266454de0, flags=57346, parent=<value optimized out>, child_role=<value optimized out>, errp=0x7fffbcd44468) at block.c:1104 4 bdrv_open_inherit (filename=<value optimized out>, reference=<value optimized out>, options=0x7f5266454de0, flags=57346, parent=<value optimized out>, child_role=<value optimized out>, errp= 0x7fffbcd44468) at block.c:1833 5 0x00007f5264cbd4d9 in bdrv_open_child (filename=0x7fffbcd46141 "vxhs://127.0.0.1:9999/t.raw", options=0x7f526644e980, bdref_key=0x7f5264dc9ab0 "file", parent=0x7f526644a720, child_role= 0x7f5264ffe5e0, allow_none=true, errp=0x7fffbcd44468) at block.c:1588 6 0x00007f5264cbbf0b in bdrv_open_inherit (filename=0x7fffbcd46141 "vxhs://127.0.0.1:9999/t.raw", reference=<value optimized out>, options=0x7f526644e980, flags=24578, parent=<value optimized out>, child_role=<value optimized out>, errp=0x7fffbcd44548) at block.c:1794 7 0x00007f5264cbcd83 in bdrv_open (filename=<value optimized out>, reference=<value optimized out>, options=<value optimized out>, flags=<value optimized out>, errp=<value optimized out>) at block.c:1924 8 0x00007f5264cff7fc in blk_new_open (filename=0x7fffbcd46141 "vxhs://127.0.0.1:9999/t.raw", reference=0x0, options=0x7f5266449700, flags=16386, errp=<value optimized out>) at block/block-backend.c:160 9 0x00007f5264cb3151 in openfile (name=0x7fffbcd46141 "vxhs://127.0.0.1:9999/t.raw", flags=<value optimized out>, writethrough=false, opts=0x7f5266449700) at qemu-io.c:67 10 0x00007f5264cb3a64 in main (argc=<value optimized out>, argv=0x7fffbcd449c8) at qemu-io.c:604

Fix segfault in iio_open()

Increase vdisk_path size since it can easily overflow if the test server is started with a long directory path, as is the case while running qemu-iotests.

Increase vdisk_path size.

The "devid" input argument to iio_open() is used in iio_read_hostinfo() to compose the filename string, which is a fixed sized char array. This means that the client caller is trusted to not pass a string that will result in an out-of-bound memory write. Validate the input size (I arbitrarily chose the size of 256, determined conservatively from the client code in the proposed QEMU driver that caps the size to 37 bytes). This will now exit with error in the iio_open() if devid size exceeds the limits, and in the iio_read_hostinfo() call the string copy is made safe to make sure the composed string never exceeds the array size.

Make sure input argument "devid" to iio_open is bounded

(1) Use snprintf to prevent exceeding the size of dest string. (2) Add a check to error out if the combined length of destination directory, and the file being opened, exceed what can be stored.

Fix possible buffer overflows from target dir and file being opened

Account for the '/' that we add between the directory and file name agruments while validaing their combined length.

Account for the '/' that we add between the directory and file name

stefanha

Fixed-length string buffers are awkward. In the future you could consider using variable-length strings internally and enforcing a length limit during input validation. That means working with char * instead of fixed-size arrays.

stefanha · 2017-02-13T10:05:41Z

src/lib/qnio/iioapi.c

@@ -481,7 +481,7 @@ iio_open(const char *uri, const char *devid, uint32_t flags)
    if (hostinfo == NULL) {
        nioDbg("Unable to read the host information for device %s\n", devid);
        hostinfo = (struct iio_vdisk_hostinfo *)malloc(sizeof (struct iio_vdisk_hostinfo));
-        strncpy(hostinfo->hosts[hostinfo->nhosts], uri, NAME_SZ);
+        strncpy(hostinfo->hosts[0], uri, NAME_SZ);


A pre-existing problem:

This strncpy() call is unsafe. If uri is at least NAME_SZ characters long then ->hosts[0] will lack the NUL-terminator. Later calls to string functions may access memory beyond the end of ->hosts[0].

Please use safe_strncpy() everywhere in the codebase.

Fixed in the latest patch on securify branch.

stefanha · 2017-02-13T10:16:41Z

src/test/server.c

+    if (strlen(bname) + strlen(vdisk_dir) > DIR_NAME_SZ - 1) {
+        fprintf(stderr, "Combined length of directory path and "
+                "filename exceeds %d.\n", DIR_NAME_SZ - 1);
+        exit(-1);


POSIX says "only the least significant 8 bits (that is, status & 0377) shall be available to a waiting parent process":
http://pubs.opengroup.org/onlinepubs/000095399/functions/exit.html

Error status codes are positive. Please use EXIT_FAILURE instead of -1.

stefanha · 2017-02-13T10:37:51Z

src/lib/qnio/nio_server.c

+                                event.data.ptr = ep;
+                                s = epoll_ctl(qns_ctx->epoll_fd, EPOLL_CTL_ADD, infd, &event);
+                                if (s == -1) {
+                                    nioDbg("epoll_ctl error");


Missing SSL_free(ssl) and free(ep).

Fixed in latest patch on securify branch.

stefanha · 2017-02-13T10:39:53Z

src/lib/qnio/nio_server.c

+                    if (sslerr == SSL_ERROR_WANT_READ || sslerr == SSL_ERROR_WANT_WRITE) 
+                    {
+                        nioDbg("ssl accept still not complete");
+                        continue;


The fd was deleted from epoll so future read/write events will not be processed and the connection is stalled. Did I misread the code?

We need to keep processing events on this fd.

You are right. EPOLL_CTL_DEL needs to happen after successful SSL_accept. Will fix this.

Fix checked in the latest patch on securify branch.

stefanha · 2017-02-13T10:40:24Z

src/lib/qnio/nio_server.c

+                    {
+                        nioDbg("ssl accept failed %s", ERR_error_string(ERR_get_error(), NULL));
+                        SSL_free(ssl);
+                        if (ep) free(ep);


free(NULL) is a nop so it's checking for NULL is unnecessary.

Fixed in latest patch on securify branch.

stefanha · 2017-02-13T10:44:40Z

src/lib/qnio/nio_server.c

+                            {
+                                nioDbg("SSL_error is want read or want write %d", ret);
+                                /* put fd in epoll loop to try again */
+                                event.events = EPOLLIN;


OpenSSL may be waiting for space to send data so ret needs to be checked:
SSL_ERROR_WANT_READ -> EPOLLIN
SSL_ERROR_WANT_WRITE -> EPOLLOUT

Agreed. Will make the change.

Fix checked in on the latest patch for securify branch.

berrange · 2017-02-13T15:05:12Z

src/lib/qnio/utils.c

+int 
+is_secure()
+{
+    if (access(SECURE_IMPL, F_OK) != 0)


Turning on / off use of SSL based on existence of a random path on host is awful behaviour.

This needs to be under direct control via the API. ie, QEMU must be able to instruct libqnio whether or not to use SSL for each connection made, without side effects from host OS state.

How would this work if this was driven by QEMU? I assume somewhere on the host there would be a VxHS configuration file which would indicate whether to use SSL or not. This would be set to what the VxHS user has chosen. Would it be better if libvxhs uses a configuration file say /etc/vxhs.conf for this?
Also if the choice is driven by the VxHS user and the implementation is in libvxhs then why does QEMU have to have control over this?

QEMU aims to avoid guest config relying on external configuration files because that makes part of the setup invisible to the management tools. Instead all relevant parameters that influence operation of QEMU are explicitly configured via its command line. Compare with the RBD block device - while librbd has ability to use a config file in /etc/, for example to set passwords, QEMU will explicitly take the passwords on the command line instead of using the config file, so the mgmt app can be in complete control.

This allows the mgmt application to fully control QEMU operation, without having to rely on someone having manually edited a configuration file on the host previously. So QEMU should not have any dependancy on a /etc/vxhs.conf file - all aspects should be configurable via QEMU options, and thus exposed upto the libvirt management system, and in turn exposed to apps.

The libqnio API for starting a session would really be better refactored. It currently is

void *iio_open(const char *uri, const char *devid, uint32_t flags);

which both creates the handle and opens the session. I'd suggest this needs splitting. For example

void *iio_new();
int32_t iio_open(void *dev_handle, const char *uri, const char *devid, uint32_t flags);

Then you can add

iio_set_tls(void *dev_handle, const char *capath, const char *cacrl, const char *certpath, const char *keypath);

The application would call iio_set_tls after iio_new(), but before iio_open in order to explicitly request use of TLS on that handle.

IMO, having the qemu management layer exercise complete control over the block device characteristics and operations is a noble but non-trivial goal. There will always be certain aspects of the underlying device that need not be known to QEMU.
E.g.

failover target information for vxhs vdisks.

number of retries before failover
Are these something that you think needs to be driven through QEMU as well? That would mean that we need to add APIs like iio_set_failover_targets() API to pass this down from qemu to the block device handler library. This BTW, was earlier being passed through libvirt but was changed to be handled locally by libvxhs after reviews from community.
Relying solely on qemu/libvirt mgmt interface to tune each and every one of the block device parameters would not be an efficient way to manage the block device.

Use of ssl/tls (if the user decides to enable it) is a global setting for VxHS block devices. Our use case doesn't demand this setting per instance. If VxHS determines this to be a useful setting on a per instance basis, then we will add the new API and the necessary management layer changes at a future date.

Also, it would really help if these requirements were published and maintained as guidelines for future drivers/contributions.

This is all general best practice for library design. Library code in general should not rely on configuration files to determine its behaviour as different concurrent users of the library may wish to have different configuration settings. Use of external configuration files should be delegated to users of the library, or if the library does wish to have some defaults loaded from config files, it should always allow those defaults to be overridden via API calls.

berrange · 2017-02-13T15:08:09Z

src/lib/qnio/utils.c

+    strcpy(clientcert, CLIENT_KEYSTORE);
+    strncat(clientcert, instanceid, 64);
+    strncat(clientcert, ".cert", 5);
+


These paths need to be made configurable via the libqnio API. QEMU needs to be able to provide the path for the client cert & key PEM files.

We also need to be able to set a custom CA certificate bundle - allowing Openssl to use the global CA trust database is very undesirable. The global trust DB contains 100's of CA certs which are of no benefit for private network deployments. In fact using system trust DB reduces security, because any one of those CAs could be compromised by attackers allowing them to issue bogus certs. The libqnio should be able to only ever use the specific CA configured by the organization deploying it. Again the public CA bundle path needs to be configurable by QEMU via the API.

Again, why does QEMU need to be able to provide these paths if libvxhs can reliably read these from a configuration file say /etc/vxhs.conf?
Is the keystore going to change for different clients?
Regarding the custom CA bundle, this can be an option in /etc/vxhs.conf as well.

Both the read and write functions are using a static global file pointer. This means that the pointer gets clobbered during multi-threaded operation. Since the referenced file pointer is opened, operated on, and closed all in the same function call, there is no reason for it to not be scoped to the function. (This was noticed because when running qemu with a raw image on the test-server, the test-server would abort() on a double-free).

test server: fix double-free on file pointer.

stefanha · 2017-02-15T10:09:17Z

src/lib/qnio/nio_server.c

@@ -452,14 +452,16 @@ qns_server_start(char *node, char *port)
                            {
                                nioDbg("SSL_error is want read or want write %d", ret);
                                /* put fd in epoll loop to try again */
-                                event.events = EPOLLIN;
+                                event.events = EPOLLIN | EPOLLOUT;


This causes the event loop to spin, burning CPU. Imagine OpenSSL is waiting to receive data. EPOLLOUT causes epoll_wait() to return immediately because the socket is writable. OpenSSL won't be able to make any progress though since it's waiting to receive (not send). Therefore this change causes the process to consume CPU.

As I wrote in the previous review comment: SSL_ERROR_WANT_READ -> EPOLLIN, SSL_ERROR_WANT_WRITE -> EPOLLOUT.

Checked in a new patch which sets EPOLLIN for WANT_READ and EPOLLOUT for WANT_WRITE. Surprisingly I am always getting a EPOLLIN for the fd on incomplete ssl accept. I never see an EPOLLOUT.

EPOLLOUT should be rare. It occurs when the socket sndbuf is exhausted - then the kernel network stack has to tell the application to hold off until space becomes available again.

WANT_READ and WANT_WRITE

This will be called by the new qemu patch (v9) on the last close.

Return zero-filled read buffers upto to the requested block size

MittalAshish · 2017-02-16T18:55:42Z

Merging SSL and other review changes to master.

ketonne requested review from abhishek-kane, madhav-buddhi, MittalAshish and rakeshIranjan January 29, 2017 03:35

cuhler added 2 commits February 1, 2017 11:40

Changed library name to libvxhs.so

dbfc9a8

Modified test server to restore daemonization code

097cae3

stefanha reviewed Feb 2, 2017

View reviewed changes

stefanha suggested changes Feb 2, 2017

View reviewed changes

berrange reviewed Feb 2, 2017

View reviewed changes

cuhler and others added 3 commits February 3, 2017 15:09

Fixes to address review comments by Stefan H.

368702c

Changed instance to const char *

a263937

Initialize instance id in unsecure mode as well

5c459fc

codyprime suggested changes Feb 7, 2017

View reviewed changes

codyprime reviewed Feb 7, 2017

View reviewed changes

MittalAshish and others added 11 commits February 8, 2017 18:59

Fixes for some issues that were breaking qemu-io test.

da72f0d

(1) Open file in correct mode. (2) Close the file when done so as to reset the file pointer back to zero offset.

1. Changed paths to reflect new lib name

3db8995

2. Removed instance id from message header 3. Made SSL accept non-blocking 4. Addressed a couple of leaks

Merge branch 'securify' of https://github.com/VeritasHyperScale/libqnio…

4b76f75

… into securify

Merge pull request #13 from VeritasHyperScale/ashish_securify_changes

1a57a3f

Fixes for some issues that were breaking qemu-io test.

Removed authorization check

cb3ba1c

Merge branch 'securify' of https://github.com/VeritasHyperScale/libqnio…

87a7999

… into securify

Merge pull request #14 from VeritasHyperScale/ashish_securify

9009eb1

Fix segfault in iio_open()

Increase vdisk_path size.

ab229cf

Increase vdisk_path size since it can easily overflow if the test server is started with a long directory path, as is the case while running qemu-iotests.

Merge pull request #15 from VeritasHyperScale/ashish_securify

80ef40a

Increase vdisk_path size.

MittalAshish added 5 commits February 9, 2017 19:05

Merge pull request #16 from codyprime/jtc

4a13aa8

Make sure input argument "devid" to iio_open is bounded

Fix possible buffer overflows from target dir and file being opened

a30d22e

(1) Use snprintf to prevent exceeding the size of dest string. (2) Add a check to error out if the combined length of destination directory, and the file being opened, exceed what can be stored.

Merge pull request #17 from VeritasHyperScale/ashish_securify

ac9e927

Fix possible buffer overflows from target dir and file being opened

Account for the '/' that we add between the directory and file name

3787008

Account for the '/' that we add between the directory and file name agruments while validaing their combined length.

Merge pull request #18 from VeritasHyperScale/ashish_securify

7050594

Account for the '/' that we add between the directory and file name

stefanha reviewed Feb 13, 2017

View reviewed changes

stefanha suggested changes Feb 13, 2017

View reviewed changes

berrange suggested changes Feb 13, 2017

View reviewed changes

codyprime and others added 2 commits February 13, 2017 10:50

Merge pull request #19 from codyprime/jtc

ae24d10

test server: fix double-free on file pointer.

stefanha approved these changes Feb 13, 2017

View reviewed changes

MittalAshish and others added 3 commits February 13, 2017 23:24

Return zero-filled read buffers upto to the requested block size

879f525

Review comments from Stefan on 02/12

5e697e3

Check for EPOLLOUT while completing ssl accept

84d7be4

stefanha suggested changes Feb 15, 2017

View reviewed changes

knilangekar and others added 2 commits February 15, 2017 13:22

Setting EPOLLIN and EPOLLOUT events based on ssl error values

17d6b15

WANT_READ and WANT_WRITE

Add iio_fini() to qnio_api.h

b52c877

This will be called by the new qemu patch (v9) on the last close.

stefanha approved these changes Feb 16, 2017

View reviewed changes

Merge pull request #20 from VeritasHyperScale/ashish_securify

bc5bf2d

Return zero-filled read buffers upto to the requested block size

MittalAshish merged commit e13990d into master Feb 16, 2017

Implementation of SSL in libqnio #12

Implementation of SSL in libqnio #12

Conversation

ketonne commented Jan 29, 2017

stefanha Feb 2, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

berrange left a comment

Choose a reason for hiding this comment

stefanha commented Feb 4, 2017

codyprime left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stefanha left a comment

Choose a reason for hiding this comment

stefanha Feb 13, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MittalAshish commented Feb 16, 2017

stefanha Feb 2, 2017 •

edited

codyprime left a comment •

edited

stefanha Feb 13, 2017 •

edited