Permalink
Browse files

Polishes authentication support

- Libvirt drivers release memory allocated
for username and password strings. We are using strdup to duplicate
the strings and allow Libvirt drivers to free the  memory.

- segfault-handler module was added to aid development by
printing more detailed information when segfaults occur.

- Removes use of C++ standard library to keep the node-libvirt code
consistent with Node.js core code.

- Uses Hypervisor own instance to temporary store username and password
as opposed to creating a new class for those properties only.

- Makes authentication callback a private member of the Hypervisor class
to keep consistency with the other callback functions present in this class.

Authencation was tested against VMware VSphere 5.5 but commented in the tests
as Libvirt Test driver does not support this function.
  • Loading branch information...
1 parent 9b319de commit 341d40a971ea322a51c25c4c5bc2d6ed32fc7b4b @c4milo c4milo committed Dec 24, 2013
View
@@ -1,3 +1,4 @@
build
.DS_Store
-*.sublime-*
+*.sublime-*
+node_modules
View
7 API.md
@@ -2,15 +2,14 @@
## Hypervisor
-### new Hypervisor(options)
+### new Hypervisor(uri, options)
Options:
- - uri: libvirt connection URI
- user: (optional) username for authentication
- password: (optional) password for authentication
- - readOnly: (optional) wether the connection is read-only
+ - readonly: (optional) whether the connection is read-only
- If options is a string it is used as the URI.
+ If options is a boolean it is used as the readonly parameter
### getBaselineCPU([cpu1, cpu2, cpu3, cpuN])
Computes the most feature-rich CPU which is compatible with all given host CPUs.
View
@@ -1,17 +1,25 @@
{
"name" : "libvirt",
- "version": "0.1.0",
+ "version": "0.2.0",
"author": "Camilo Aguilar - http://github.com/c4milo",
"email": "camilo@cloudescape.com",
"contributors": [
- "Subho Banerjee - subs.zero [AT] gmail [DOT] com",
- "Daniel Tralamazza - daniel.tralamazza [AT] epfl [DOT] ch"
+ "Subho Banerjee - subs.zero [AT] gmail [DOT] com",
+ "Daniel Tralamazza - daniel.tralamazza [AT] epfl [DOT] ch",
+ "Wout Mertens - @wmertens",
+ "Michael Maier - https://github.com/binBASH"
],
"description": "libvirt bindings for v8 javascript engine",
"repositories": [{
"type": "git",
"url": "http://github.com/c4milo/node-libvirt.git"
}],
+
+ "devDependencies": {
+ "expresso": "*",
+ "segfault-handler": "*"
+ },
+
"main": "index.js",
"scripts": {
"test": "expresso"
View
@@ -57,7 +57,6 @@ Handle<Value> Hypervisor::name(const Arguments& args) { \
namespace NodeLibvirt {
- static Persistent<String> hypervisor_uri_symbol;
static Persistent<String> hypervisor_username_symbol;
static Persistent<String> hypervisor_password_symbol;
static Persistent<String> hypervisor_readonly_symbol;
@@ -347,7 +346,6 @@ namespace NodeLibvirt {
NODE_DEFINE_CONSTANT(object_tmpl, VIR_DOMAIN_EVENT_ID_LAST);
#endif
- hypervisor_uri_symbol = NODE_PSYMBOL("uri");
hypervisor_username_symbol = NODE_PSYMBOL("username");
hypervisor_password_symbol = NODE_PSYMBOL("password");
hypervisor_readonly_symbol = NODE_PSYMBOL("readonly");
@@ -392,137 +390,120 @@ namespace NodeLibvirt {
}
- static int myVirConnectAuthCallback(virConnectCredentialPtr cred,
- unsigned int ncred,
- void *cbdata) {
- size_t i;
- ConnData *cd = (ConnData *)cbdata;
-
- for (i = 0; i < ncred; i++) {
- switch (cred[i].type) {
- case VIR_CRED_USERNAME:
- case VIR_CRED_AUTHNAME:
- case VIR_CRED_ECHOPROMPT:
- case VIR_CRED_REALM:
- // Put username in cred[i].result
- if (cd->user.length()) {
- cred[i].resultlen = cd->user.length();
- cred[i].result = (char *) malloc(cred[i].resultlen);
- strcpy(cred[i].result, cd->user.c_str());
- } else {
- cred[i].resultlen = 0;
- }
- break;
-
- case VIR_CRED_PASSPHRASE:
- case VIR_CRED_NOECHOPROMPT:
- // Put password in cred[i].result
- if (cd->password.length()) {
- cred[i].resultlen = cd->password.length();
- cred[i].result = (char *) malloc(cred[i].resultlen);
- strcpy(cred[i].result, cd->password.c_str());
- } else {
- cred[i].resultlen = 0;
- }
- break;
+ Hypervisor::Hypervisor( char* uri,
+ char* username,
+ char* password,
+ bool readonly)
+ : ObjectWrap() {
- default:
- return -1;
- }
- }
-
- return 0;
- }
- static int myVirConnectCredTypes[] = {
- VIR_CRED_AUTHNAME,
- VIR_CRED_PASSPHRASE,
- };
-
- ConnData::ConnData(const std::string& uri_,
- const bool readOnly_,
- const std::string& user_,
- const std::string& pass_)
- : uri(uri_), readOnly(readOnly_), user(user_), password(pass_) {
- }
+ static int supported_cred_types[] = {
+ VIR_CRED_AUTHNAME,
+ VIR_CRED_PASSPHRASE,
+ };
- Hypervisor::Hypervisor(const std::string& uri,
- bool readOnly,
- const std::string& user,
- const std::string& pass)
- : ObjectWrap(), connData(uri, readOnly, user, pass) {
- HandleScope scope;
+ this->username_ = username;
+ this->password_ = password;
- virConnectAuth thisVirConnectAuth = {
- myVirConnectCredTypes,
- sizeof(myVirConnectCredTypes)/sizeof(int),
- &myVirConnectAuthCallback,
- (void *)&connData
- };
+ virConnectAuth auth;
+ auth.credtype = supported_cred_types; //declared and initialized in hypervisor.h
+ auth.ncredtype = sizeof(supported_cred_types)/sizeof(int);
+ auth.cb = Hypervisor::auth_callback;
+ auth.cbdata = this;
- conn_ = virConnectOpenAuth(connData.uri.c_str(), &thisVirConnectAuth,
- connData.readOnly ? VIR_CONNECT_RO : 0);
+ conn_ = virConnectOpenAuth( (const char*) uri,
+ &auth,
+ readonly ? VIR_CONNECT_RO : 0);
if(conn_ == NULL) {
ThrowException(Error::New(virGetLastError()));
}
}
+ int Hypervisor::auth_callback( virConnectCredentialPtr cred,
+ unsigned int ncred,
+ void *data) {
+ Hypervisor *hyp = static_cast<Hypervisor*>(data);
+
+ for (unsigned int i = 0; i < ncred; i++) {
+ if (cred[i].type == VIR_CRED_AUTHNAME) {
+ cred[i].result = hyp->username_;
+
+ if (cred[i].result == NULL) {
+ return -1;
+ }
+ cred[i].resultlen = strlen(cred[i].result);
+ } else if (cred[i].type == VIR_CRED_PASSPHRASE) {
+ cred[i].result = hyp->password_;
+ if (cred[i].result == NULL) {
+ return -1;
+ }
+ cred[i].resultlen = strlen(cred[i].result);
+ }
+ }
+
+ return 0;
+ }
+
virConnectPtr Hypervisor::connection() const {
return conn_;
}
Handle<Value> Hypervisor::New(const Arguments& args) {
HandleScope scope;
- std::string uriStr;
- std::string userStr;
- std::string pwStr;
- bool readOnly = false;
- if(args.Length() == 0 ) {
+ bool readonly = false;
+ char* uri = NULL;
+ char* username = NULL;
+ char* password = NULL;
+
+ int argsLen = args.Length();
+
+ if (argsLen == 0) {
return ThrowException(Exception::TypeError(
- String::New("You must specify connection options")));
+ String::New("You must specify a connection URI as first argument")));
}
- if (args[0]->IsString()) {
- String::Utf8Value t0(args[0]->ToString());
- uriStr = std::string(*t0);
+ if (argsLen == 1 && !args[0]->IsString()) {
+ return ThrowException(Exception::TypeError(
+ String::New("You must specify a string as connection URI")));
+ }
- if(args.Length() == 2 && !args[1]->IsBoolean()) {
- return ThrowException(Exception::TypeError(
- String::New("Second argument must be a boolean")));
- }
+ Local<String> uriStr = args[0]->ToString();
+ String::Utf8Value uri_(uriStr);
+ uri = *uri_;
- readOnly = args[1]->IsTrue();
- } else if (args[0]->IsObject()) {
- Local<Object> arg_obj = args[0]->ToObject();
- if( !arg_obj->Has(hypervisor_uri_symbol) ||
- !arg_obj->Get(hypervisor_uri_symbol)->IsString()) {
- return ThrowException(Exception::TypeError(
- String::New("Need uri to connect")));
- }
- String::Utf8Value t1(arg_obj->Get(hypervisor_uri_symbol)->ToString());
- uriStr = std::string(*t1);
+ if (argsLen >= 2) {
+ if (args[1]->IsBoolean()) {
+ readonly = args[1]->IsTrue();
+ } else if (args[1]->IsObject()) {
+ Local<Object> args_ = args[1]->ToObject();
- if( arg_obj->Has(hypervisor_username_symbol) ) {
- String::Utf8Value t2(arg_obj->Get(hypervisor_username_symbol)->ToString());
- userStr = std::string(*t2);
- }
- if( arg_obj->Has(hypervisor_password_symbol) ) {
- String::Utf8Value t3(arg_obj->Get(hypervisor_password_symbol)->ToString());
- pwStr = std::string(*t3);
- }
- if( arg_obj->Has(hypervisor_readonly_symbol) ) {
- readOnly = arg_obj->Get(hypervisor_readonly_symbol)->IsTrue();
+ if (args_->Has(hypervisor_readonly_symbol) &&
+ args_->Get(hypervisor_readonly_symbol)->IsBoolean()) {
+ readonly = args_->Get(hypervisor_readonly_symbol)->IsTrue();
+ }
+
+ if (args_->Has(hypervisor_username_symbol)) {
+ String::Utf8Value username_(args_->Get(hypervisor_username_symbol));
+ //We need to use strdup because the driver
+ //is attempting to release cred[i].result memory for us.
+ username = strdup(*username_);
+ }
+
+ if (args_->Has(hypervisor_password_symbol)) {
+ String::Utf8Value password_(args_->Get(hypervisor_password_symbol));
+ //We need to use strdup because the driver
+ //is attempting to release cred[i].result memory for us.
+ password = strdup(*password_);
+ }
}
- } else {
- return ThrowException(Exception::TypeError(
- String::New("Please provide connection options as an object")));
}
- Hypervisor *hypervisor = new Hypervisor(uriStr, readOnly, userStr, pwStr);
+
+ Hypervisor *hypervisor = new Hypervisor(uri, username, password, readonly);
Local<Object> obj = args.This();
hypervisor->Wrap(obj);
- return obj;
+ return scope.Close(obj);
}
Handle<Value> Hypervisor::GetCapabilities(const Arguments& args) {
View
@@ -16,18 +16,6 @@
namespace NodeLibvirt {
- class ConnData {
- public:
- std::string uri;
- const bool readOnly;
- std::string user;
- std::string password;
- ConnData( const std::string& uri,
- const bool readOnly,
- const std::string& user,
- const std::string& pw);
- };
-
class Hypervisor : public ObjectWrap {
public:
static void Initialize(Handle<Object> target);
@@ -40,6 +28,7 @@ namespace NodeLibvirt {
//return constructor_template->HasInstance(object);
}
virConnectPtr connection() const;
+
protected:
static Handle<Value> New(const Arguments& args);
@@ -96,15 +85,16 @@ namespace NodeLibvirt {
//Misc functions
static Handle<Value> FindStoragePoolSources(const Arguments& args);
- Hypervisor( const std::string& uri,
- bool readOnly,
- const std::string& userStr,
- const std::string& pwStr);
+ Hypervisor( char* uri,
+ char* user,
+ char* pass,
+ bool readOnly);
private:
virConnectPtr conn_;
- ConnData connData;
+ char* username_;
+ char* password_;
static void domain_event_free(void *opaque);
static int domain_event_lifecycle_callback( virConnectPtr conn,
@@ -144,6 +134,9 @@ namespace NodeLibvirt {
const char *authScheme,
virDomainEventGraphicsSubjectPtr subject,
void *opaque);
+ static int auth_callback( virConnectCredentialPtr cred,
+ unsigned int ncred,
+ void *data);
};
} // namespace NodeLibvirt
View
@@ -1,5 +1,7 @@
-// Copyright 2010, Camilo Aguilar. Cloudescape, LLC.
-#define BUILDING_NODE_EXTENSION
+// Copyright 2013, Camilo Aguilar. Cloudescape, LLC.
+#ifndef BUILDING_NODE_EXTENSION
+ #define BUILDING_NODE_EXTENSION
+#endif
#include <stdio.h>
#include <stdlib.h>
#include "node_libvirt.h"
@@ -49,4 +51,4 @@ namespace NodeLibvirt {
NODE_MODULE(libvirt, init)
} //namespace NodeLibvirt
-
+
View
17 test.js
@@ -0,0 +1,17 @@
+'use strict';
+
+var SegfaultHandler = require('segfault-handler');
+SegfaultHandler.registerHandler();
+
+var libvirt = require('./build/Release/libvirt');
+
+var Hypervisor = libvirt.Hypervisor;
+
+var hypervisor = new Hypervisor('esx://172.16.103.128/?no_verify=1', {
+ username: 'root',
+ password: 'shadow04101',
+ readOnly: false
+});
+
+console.log(hypervisor.getVersion());
+console.log(hypervisor.getCapabilities());
Oops, something went wrong.

3 comments on commit 341d40a

Contributor

wmertens replied Dec 24, 2013

Nice 👍

One comment though: Once the authentication function passes the credentials to libvirt, they will be freed, so perhaps they should be set to NULL in the hypervisor class?

Owner

c4milo replied Dec 24, 2013

Yes, you are totally right. Thank you for pointing that out @wmertens

Owner

c4milo replied Dec 24, 2013

I just had 5 min to review again and I believe the code is currently correct. When Hypervisor::New receives the credentials from Javascript land, they are assigned to local v8 handlers that should be marked to be GC'd once the Hypervisor::New function finishes. The copies being passed in https://github.com/c4milo/node-libvirt/blob/master/src/hypervisor.cc#L502, as you already know, will get freed eventually by the Libvirt drivers.

I hope this make sense. Please, let me know if I'm overlooking something.

Please sign in to comment.