Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add ability to pass a custom sign function to signRequest #58

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

lamara
Copy link

@lamara lamara commented May 4, 2016

This lets you explicitly specify how to sign a signRequest signature string via a passed in function. Useful if you would like to use an arbitrary signing method with the signRequest API -- one specific use case being the offloading of sign requests to an SSM or HSM (keeping private key data secure), instead of having to pass the private key explicitly into the signRequest function.

Because this was designed to be able to sign things off-process (like with an HSM), the passed in function is asynchronous, meaning a callback function must be passed in to signRequest if a signing function is specified. This new functionality does not have any effect on existing API.

@arekinath
Copy link
Contributor

So, this is basically what the RequestSigner API is for. Did you have a look at that at all? I know it's lacking documentation and examples at the moment, but it already provides for async signing using a callback function (in fact the whole reason it was added is because we use the ssh-agent to sign requests). An example looks like:

const mod_http = require('http');
const mod_httpsig = require('http-signature');

var signer = mod_httpsig.createSigner({
    sign: signFunc
});
var date = signer.writeDateHeader();
signer.sign(function (err, authz) {
    if (err) { ...; return; }
    var req = mod_http.request(httpOptions, function (res) {
        ...
    });
    req.setHeader('Date', date);
    req.setHeader('Authorization', authz);
    req.end();
});

function signFunc(data, cb) {
    /* do some things to generate a signature on "data" (a String) */
    var signaturebase64 = ...;
    cb(null, {
        keyId: 'somekeyid',
        algorithm: 'rsa-sha256',
        signature: signaturebase64
    });
}

This API is meant to give you fine-grained control over how the request is signed so you can easily tie it into whatever SSM/HSM you want to use and whatever HTTP request library or format you like (it can even let you re-use the http-signature concept on things that aren't HTTP).

@dlongley
Copy link

dlongley commented May 4, 2016

@arekinath,

This patch was intended to make it easy to add a follow up PR to the request library to fix both a bug (you sometimes can't sign content-length using that library at present because it is calculated too late) and to enable providing a sign function in a simple way through the options parameter:

digitalbazaar/request@f3cdbc2

This seemed like the easiest way to expose the capability to sign asynchronously with a custom method without any other changes to the common API. What do you think?

@arekinath
Copy link
Contributor

In that code in request, it has to create an adapter object (a fake http request object) to feed to httpSignature, which is really not great -- the spec for what those should look like is not controlled by http-signature at all (and isn't even really documented in node; the whole thing is not a public interface to start with). If for some reason upstream node decides to change it (in a major rev, which they would be perfectly entitled to do), we end up in a bit of a pickle: do we adjust this function to take the new format and potentially have to break old clients of the library, or do we keep compat with old clients and break the promise that we can take a node http object? Which is why I actually would like to move away from it as being the "main" API of the library -- hence createSigner and the RequestSigner object.

At this point I actually consider the signRequest function to be legacy API that I would like to keep only for compatibility in future -- the only reason it's not just a short wrapper around calls to RequestSigner right now is that it has to return synchronously. I'm going to be updating the documentation for this library to reflect that sometime soon (and adding better documentation at the same time, hopefully, as at the moment it's a bit lacking).

One other aside that's kind of important to mention is that some SSM/HSMs can't necessarily give you what hash algorithm they're going to use in a signature ahead of time. This is particularly the case with the SSH agent, but also some hardware tokens (which reserve the right to force you to use a different hash as they or their firmware get upgraded). So I'm not totally sold on an API requiring that the algorithm be specified before any signing takes place, either.

@dlongley
Copy link

dlongley commented May 4, 2016

@arekinath,

So would your recommendation then be to just submit a PR to request using the RequestSigner API instead?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants