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: https://github.com/kr1sp1n/node-vault/pull/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:

• [x] Use ES6 syntax
• [x] ~Provide packages for different Auth backends~ (wontfix)
• [x] 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:

• [x] Use ES6 syntax
• [x] ~Provide packages for different Auth backends~ (won't do)
• [x] Remove mustache
• [x] ~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