Modernize implementation

[StarpTech]

As titled. Any plans yet?

• Use Tagged template literals instead mustache
• Use r2 instead of full-blown request + request-native-promise package
• Use ES6 syntax
• Provide packages for different Auth backends
• Provide better documentation

I will help you to reach the goals.

[kr1sp1n]

Hi @StarpTech, a few days before your proposals I came up with this #75
Nice coincidence 😉 So I would love to start to modernize this nice little package and appreciate your help. Do you use node-vault for production atm?

[DaniGuardiola]

Hi! I'm currently working on this: #78

I've experimented a bit with tagged templates and I've arrived at the conclusion that the use of mustache comes in handy because it allows for a consistent and declarative code, and is also very readable, making the generation of documentation (through scripts/write-features.js) very easy and convenient. I would leave it as it is, it works pretty well :)

I've transformed pretty much everything into modern ES6 code with standardjs styleguide. I still need to dig a bit deeper in some of the module methods but it's pretty much done.

I'm gonna investigate r2 now and see why and how to implement it into the project.

About splitting the project, I'm not sure that would serve any purpose really. The methods, as they are defined now, take little to no space in the project and the memory, and I don't see why a client would bother adding separate pieces of functionality at different times when you can just access to the whole HTTP API through node-vault. I think it would come as confusing, just my two cents.

About the documentation, I'm almost finished documentation the code for devs (using the docco setup already in place) and I plan on adding the documentation package and a package.json script, to generate extensive documentation for some of the relevant methods on the Vault class, which is now where the module logic resides. This documentation will only be useful for developers and code reviewers, but it's important nonetheless as this is a security-related open-source project.

I'm also thinking on adding some more intensive tests, like reading, writing, deleting secrets, creating users, policies, checking them, etc, some random features to cover a good amount of functionality.

After this 'refactor' project is done, I will implement AIM and EC2-AIM auth (and maybe EC2 and lambda as well) auth as well as some helper interfaces. I'm thinking on exposing an automatically aws authenticated method, something like:

const vault = require('node-vault').awsAuth()

vault.read(...)
vault.write(...)

This way there's no hassle for developers (other than setting up the VAULT_ADDR env variable on EC2 instances). This helper interface would return an authenticated client that ideally automatically detects the environment (ec2 instance or lambda function), retrieves the needed credentials and logs into vault, setting up the token automagically, so its ready to use out of the box as long as the environment is AWS (should also run well in docker containers with network access).

I would love to hear you thoughts!

Cheers,
Dani

[DaniGuardiola]

PD1: the codecov stuff is failing in my PR, help!
PD2: about splitting the project, I would keep everything on the same repo and have a complete package as default export. However I don't see a problem on exposing alternative interfaces that are either skimmed down or serve special purposes like the awsAuth helper I proposed.

[DaniGuardiola]

I've investigated r2 and I don't see any significant advantage. request is not outdated and r2 only provides a significant performance improvement in the browser. I don't think the syntax differences are worth it, our case is actually very simple and request works just fine. I would like to see some examples of how you would change the code with r2 to get a better idea.

[StarpTech]

Hi @DaniGuardiola great to see progress here.

I've experimented a bit with tagged templates and I've arrived at the conclusion that the use of mustache comes in handy because it allows for a consistent and declarative code, and is also very readable, making the generation of documentation (through scripts/write-features.js) very easy and convenient. I would leave it as it is, it works pretty well :)

I don't want to avoid the usage of mustache for template generating but for url building here that's a great use-case for template-literals.

I'm gonna investigate r2 now and see why and how to implement it into the project.
About splitting the project, I'm not sure that would serve any purpose really. The methods, as they are defined now, take little to no space in the project and the memory, and I don't see why a client would bother adding separate pieces of functionality at different times when you can just access to the whole HTTP API through node-vault. I think it would come as confusing, just my two cents.

In request + request-native-promise I see lots of code which isn't needed anymore for the new node versions. I understand your arguments and we could switch to https://github.com/feross/simple-get without to include any browser polyfills and be lightweight at the same time.

[DaniGuardiola]

But we are currently using a template literal, so I don't know what you mean :P

let uri = ${client.endpoint}/${client.apiVersion}${options.path}

In request + request-native-promise I see lots of code which isn't needed anymore for the new node versions. I understand your arguments and we could switch to https://github.com/feross/simple-get without to include any browser polyfills and be lightweight at the same time.

I agree that we can probably substitute the request module with something more lightweight and modern, but if simple-get works as the name suggests, then it is limited to GET requests and node-vault needs to make also POST (and other HTTP methods) requests. If you can find a better module that suits the requirements I can implement it, but I'm focused on aws-aim auth for now.

[StarpTech]

But we are currently using a template literal, so I don't know what you mean :P

That was quite confusing :D For which case do you need mustache at this place ?

simple-get supports any method https://github.com/feross/simple-get#post-put-patch-head-delete-support

[DaniGuardiola]

I reviewed r2 again. Even though they are doing a shim for the browser fetch API in Node.js, the shim used by r2 (node-fetch) is very lightweight as well, so I changed my mind on r2, I think it is a good fit. I will refactor the VaultClient._request method soon with the change.

[DaniGuardiola]

simple-get supports any method https://github.com/feross/simple-get#post-put-patch-head-delete-support

Ahh, that's great and even more lightweight than r2, alright I'll use it instead

[DaniGuardiola]

That was quite confusing :D For which case do you need mustache at this place ?

Ok I'll explain:

// This line creates the initial URI using a template literal
// The result would be something like:
// "http://localhost:8200/v1/sys/auth/{{auth-backend}}/role/{{role-name}}"
let uri = ${client.endpoint}/${client.apiVersion}${options.path}

// Then we use mustache to make the substitution of the request parameters
// And the result is something like this:
// "http://localhost:8200/v1/sys/auth/aws/role/test-role"
uri = mustache.render(uri, options.json)

This is convenient for the reasons I wrote in this comment.

[DaniGuardiola]

Check the features.md file and you'll understand why using mustache is a really good idea, solving two problems in one:

• Path parameter documentation
• Path parameter substitution for request

[StarpTech]

I don't speak about documentation. I point into the code here:

https://github.com/kr1sp1n/node-vault/blob/master/src/index.js#L67-L69

You will start the mustache procedure on each request for what?

[DaniGuardiola]

UPDATE: outdated list

So, current progress is:

• Use ES6 syntax
• Provide packages for different Auth backends (wontfix)
• Remove mustache
• Provide better documentation (in progress)
• Switch to simple-get instead of request + request-native-promise

[DaniGuardiola]

@StarpTech I explained how and why we are using mustache in this comment

And I explained some of the advantages of using it in this other comment

Each request needs to create the path for the URI of the request by inserting the parameters. mustache is the tool that takes care of that.

[StarpTech]

Sry I don't get it. You use template-literals to build your url and then replace variables with mustache?

Why not:

  client.request = (options = {}) => {
    const valid = tv4.validate(options, requestSchema);
    if (!valid) return Promise.reject(tv4.error);

    if (typeof client.token === 'string' && client.token.length) {
      options.headers['X-Vault-Token'] = client.token;
    }
	
    const uri = `${client.endpoint}/${client.apiVersion}${options.path}`
    debug(options.method, uri);

    return rp({
     uri,
     headers: options.headers
    }).then(handleVaultResponse);
  };

[DaniGuardiola]

Btw, you guys can help me with something, I need to finish adding documentation to features.js (for features.md). You can help me by checking that file and adding the documentation for the remaining methods. Just check the style I'm using to be consistent and remember to add a TODO if there's something like no documentation found or an outdated HTTP method (check current TODOs for more information).

If you can please do it and send a PR to my branch (DaniGuardiola/node-vault - refactor) that would be great, so I can focus on the remaining features like r2 :)

[DaniGuardiola]

@StarpTech forming the url and inserting the parameters are two different things. The parameters are contained in the path.

In the request method, the parameters are passed in options.json. Example:

{
  'auth-backend': 'aws',
  'role-name: 'test-role'
}

Mustache will match any {{variable}} substrings and replace them with the correct values.

In your snippet you are not doing this substitution and, therefore, in my request example, you would be trying to send a request to "http://localhost:8200/v1/sys/auth/{{auth-backend}}/role/{{role-name}}" and it would fail.

[DaniGuardiola]

Re-read my comment and notice that there are two steps: forming the URI and inserting the parameters in the path. For the first step a template string literal works just fine, and mustache takes care of the second step.

[StarpTech]

Ok, I got it but it feels like overhead. I don't like the idea to use a template language for URI interpolation. It includes 630 lines of javascript to do such a simple thing.

[DaniGuardiola]

I mean we could create a basic mustache-like function, but I don't know how convenient that would be. Basic substitution (just replacing variables by name) is easy, however the code is also using inverted sections, a feature from mustache that allows for conditional substitution, so maybe we should leave it as it is, mustache is not that big really. If you have a proposal to replace mustache with something else, go ahead, I'm open to suggestions, but I don't think the use of mustache is a problem for now.

Example path using inverted sections:
POST /auth/{{mount_point}}{{^mount_point}}github{{/mount_point}}/login

[DaniGuardiola]

I suppose what we need to implement are defaults to replace the inverted section usage. Then we can easily create a function with a similar interface to mustache + defaults, something like:

insertParameters(uri, options.json, options.defaults)

And the defaults would need to be defined in features.js.

[DaniGuardiola]

@kr1sp1n I got a question, if there's a parameters defined in the path without a default (mustache inverted section), and the value is not present, it should fail right? Otherwise you would get empty replacements so paths could look like this and maybe fail silently or do weird stuff path/test//login

[DaniGuardiola]

For now this is the behavior I decided, not very happy with it but I think that is the current behavior with mustache anyway

      it('should not fail if value is missing without a default', done => {
        const path = '/test/{{param_1}}/test/{{param_2}}'
        const values = {
          param_1: 'test_value_1'
        }
        const result = vault.formatRequest(path, values)
        result.should.equal('/test/test_value_1/test/')
        return done()
      })

[DaniGuardiola]

Removed mustache!

Yay! :)

Thanks @StarpTech for helping me with the idea. We can cross that off the list now, pending @kr1sp1n answer to my question.

[DaniGuardiola]

Current tasks:

• Use ES6 syntax
• Provide packages for different Auth backends (won't do)
• Remove mustache
• Switch to simple-get instead of request + request-native-promise (won't do)
• Provide better documentation (in progress)
• Organize and extend unit tests
• Add detailed integration tests (acceptance tests similar to vault's)

[DaniGuardiola]

Could you guys do some code review?

[DaniGuardiola]

So I found two problems with simple-get, check the commit to see the diff and the extended description of the issue.

[DaniGuardiola]

TL;DR:

• No strictSSL support
• Ports don't work??? Integration tests fail with connect ECONNREFUSED 127.0.0.1:80

So unless we can find a lib that supports both things, we should keep using request for now

[DaniGuardiola]

https://www.npmjs.com/package/needle

This could be an alternative, supports rejectUnauthorized which is basically what strictSSL did in the request module

[DaniGuardiola]

It is not a slim package though, it might not be worth it if it's bigger or similar to request + request-promise-native in size / efficiency

[DaniGuardiola]

Due to these problems, I think the best decision is to leave request and request-promise-native as it is.

I updated the checklist up there, now we only have tasks of documentation and testing left. We should also consider if we should add babel translation to add node 6+ (or even <6) support. What do you think about that?

[DaniGuardiola]

Vault fixed this issue! hashicorp/vault#3763

However, I still need to include the workaround to support versions prior to v0.9.2.

[StarpTech]

You can enforce ssl if you pass the correct options to simple-get. Look here:

• https://github.com/tomas/needle/blob/105ad2509c40fb5f66f96cbe77a201bec3eb04ac/lib/needle.js#L29
• https://github.com/tomas/needle/blob/105ad2509c40fb5f66f96cbe77a201bec3eb04ac/lib/needle.js#L211-L217

or look in the node.js docs here https://nodejs.org/api/https.html#https_https_request_options_callback

[DaniGuardiola]

@StarpTech that's needle, not simple-get :P

[StarpTech]

yes, I know you can also pass request options in simple-get. I show you the correct configuration.

[DaniGuardiola]

@StarpTech ok, could you take care of that? You can pull my branch were I was last working on it. It is not a priority for me as I need to focus on other stuff for my company.

The tests fail (you'll need to run yarn docker-test to correctly run the integration tests) and the ssl option is missing.

simple-get branch

[DaniGuardiola]

@kr1sp1n could you accept the PR and publish a new major version? We can add the documentation and further tests and code improvements in minor versions. This is prio for my company :)

[DaniGuardiola]

@kr1sp1n actually wait because I need to finish the aws-auth stuff, I would like to integrate it into the next major version so that we can use it. You can still accept the refactor PR and I'll send a different one with the aws-auth stuff for clarity.

[StarpTech]

I will try to help you as soon I have some time but right now I'm very busy :/

[DaniGuardiola]

@StarpTech don't worry, it can be done in the future. I think that we should release the current version as soon as I do some more tests and change the request stuff in a minor release when it's ready. Do you agree? :)

[DaniGuardiola]

Updated checklist (excluding minor details):

• Use ES6 syntax
• Provide packages for different Auth backends (won't do)
• Remove mustache
• Added support for node v6+
• Provide better documentation (in progress)
• Organize and extend unit tests
• Add detailed integration tests (acceptance tests similar to vault's)

On hold:

• Switch to simple-get instead of request + request-native-promise

[patrick-motard]

@DaniGuardiola Where can the progress on "Provide better documentation" be found?

[DaniGuardiola]

@patrick-motard #78 which is a PR from the refactor branch in my fork: https://github.com/daniguardiola/node-vault/tree/refactor :) keep in mind that although the features are almost the same, some are new (very few) and cannot be found in the published version.

[patrick-motard]

@DaniGuardiola thank you for responding. And thank you for your work to improve the codebase. The documentation is so far my largest barrier of use. I'll take the time to look over the PR.

[kr1sp1n]

@patrick-motard What specific docs are you looking for? Maybe we could help you immediately.

[patrick-motard]

I can get specific examples soon.

Offhanded comment... I'm not able to get the example on the readme working:

var options = {
    apiVersion: 'v1', // default
    endpoint: 'http://127.0.0.1:8200', // default
    token: '1234' // optional client token; can be fetched after valid initialization of the server
};

// get new instance of the client
var vault = require("node-vault")(options);

// init vault server
vault.init({ secret_shares: 1, secret_threshold: 1 })
    .then( (result) => {
        var keys = result.keys;
        // set token for all following requests
        vault.token = result.root_token;
        // unseal vault server
        return vault.unseal({ secret_shares: 1, key: keys[0] })
    })
    .catch(console.error);

Returns:

{ RequestError: Error: connect ECONNREFUSED 127.0.0.1:8200
    at new RequestError (/home/han/code/vault/node_modules/request-promise-core/lib/errors.js:14:15)
    at Request.plumbing.callback (/home/han/code/vault/node_modules/request-promise-core/lib/plumbing.js:87:29)
    at Request.RP$callback [as _callback] (/home/han/code/vault/node_modules/request-promise-core/lib/plumbing.js:46:31)
    at self.callback (/home/han/code/vault/node_modules/request/request.js:186:22)
    at emitOne (events.js:116:13)
    at Request.emit (events.js:211:7)
    at Request.onRequestError (/home/han/code/vault/node_modules/request/request.js:878:8)
    at emitOne (events.js:116:13)
    at ClientRequest.emit (events.js:211:7)
    at Socket.socketErrorListener (_http_client.js:387:9)
  name: 'RequestError',
  message: 'Error: connect ECONNREFUSED 127.0.0.1:8200',
  cause: 
   { Error: connect ECONNREFUSED 127.0.0.1:8200
    at Object._errnoException (util.js:1022:11)
    at _exceptionWithHostPort (util.js:1044:20)
    at TCPConnectWrap.afterConnect [as oncomplete] (net.js:1182:14)
     code: 'ECONNREFUSED',
     errno: 'ECONNREFUSED',
     syscall: 'connect',
     address: '127.0.0.1',
     port: 8200 },
  error: 
   { Error: connect ECONNREFUSED 127.0.0.1:8200
    at Object._errnoException (util.js:1022:11)
    at _exceptionWithHostPort (util.js:1044:20)
    at TCPConnectWrap.afterConnect [as oncomplete] (net.js:1182:14)
     code: 'ECONNREFUSED',
     errno: 'ECONNREFUSED',
     syscall: 'connect',
     address: '127.0.0.1',
     port: 8200 },
  options: 
   { json: { secret_shares: 1, secret_threshold: 1 },
     resolveWithFullResponse: true,
     simple: false,
     strictSSL: true,
     method: 'PUT',
     path: '/sys/init',
     headers: { 'X-Vault-Token': '1234' },
     uri: 'http://127.0.0.1:8200/v1/sys/init',
     callback: [Function: RP$callback],
     transform: undefined,
     transform2xxOnly: false },
  response: undefined }

Seems identical to an issue reported by another user on stackoverflow 10 months ago.

[DaniGuardiola]

@patrick-motard are you running vault on that address? Can you use the REST API (or the CLI) pointing to that address without a problem?

[schematis]

Wanted to check in and see if there's been any progress here.

[patrick-motard]

@DaniGuardiola I have not been working with this codebase since participating in this issue discussion over a year ago. I would like to think that I was smart enough to have first tested that CLI worked on the local dev version of vault I was running before I attempted to use this SDK, but I do think it's the right thing to test and confirm first in addressing this issue.

I'm not in a position to test this any time soon personally, but it shouldn't be too difficult to do for whomever wants to pick this up and work on it.

[aviadhahami]

This initiative seems stale :)
Closing for the meantime, feel free to reopen/recreate