Feature: Allow multiple Subject Alternative Name (SAN) extensions

[camsjams]

Revision of #34 and fixes #33

Changes:

• Allows a user to pass in multiple domain names for one certificate using Subject Alternative Name (SAN) extensions
• Update README with steps for using SAN feature
• Update README with steps for using Docker locally

[Js-Brecht]

Hey @camsjams, I noticed an issue with the pathForDomain() function type and calls.

[Js-Brecht]

I don't think this can be string[] here, since path.join() itself only accepts an arg array of string types.

In this context ,path.join.bind(path, domainsDir) basically becomes

(domain, ...pathSegments) => path.join(domainsDir, domain, ...pathSegments)

This is what I get in a node shell:

> configDir = '/test'
'/test'
> configPath = path.join.bind(path, configDir)
[Function: bound join]
> domainDir = configPath('domains')
'\\test\\domains'
> pathForDomain = path.join.bind(path, domainDir)
[Function: bound join]
> pathForDomain(['localhost', 'test.localhost'], 'private-key.key')
Thrown:
TypeError [ERR_INVALID_ARG_TYPE]: The "path" argument must be of type string. Received type object
    at validateString (internal/validators.js:125:11)
    at Object.join (path.js:427:7)

[Js-Brecht]

Personally, I think that using path.join.bind() like this is kind of an obscure usage, and can lead to confusion. I know it took me a moment when I first looked at it, and this is the only place I've ever seen it used.

@zetlen how would you feel about these changing to simple wrapper functions, instead of bound path.join functions?

[camsjams]

Oops - I had figured that out while testing, forgot to push - the fixed version is now here.

[zetlen]

There's a subtle issue with this. It treats the first domain in the list as the unique key for the whole list of domains. This means that if I run:

devcert.certificateFor(['friends.local'])

And then, later, I run this:

devcert.certificateFor(['friends.local', 'rachel.friends.local', 'joey.friends.local'])

This code will return the original friends.local certificate it generated first, which won't include any of those alternate names.

There's a workaround for this, which is to reorder the domain names, but that's confusing and non-obvious behavior.

I'd rather that we always sort the domain list by level (number of dots) and then lexically. THEN we use domains[0] as the unique key / filename. Then lastly, if a cert already exists, make sure that it contains all requested SANs--and if not, regenerate the cert.

This is a little more work, but the alternative is pretty surprising behavior.

Thanks again!

[camsjams]

HI @zetlen that sounds fine - I was aware of this behavior and it can lead to confusion.

One alternative and simple solution I had thought of was to instead create a numeric hash for the domain using a simple hash algorithm like:

const numericHash = (str) => {
	let hash = 5381;
	let i = str.length;

	while (i) {
		hash = hash * 33 ^ str.charCodeAt(--i);
	}

	return hash >>> 0;
};

And then passing the array into there like:

// user passes in this list
const domains = [
	'localhost',
	'local.api.example.com',
	'local.example.com',
	'local.auth.example.com'
].sort().join('');

// if domains.length = 1, we can just use first domain
const stableDomain = domains.length === 1 ? domains[0] : 'san-'+ numericHash(domains);

// later on
pathForDomain(stableDomain);

So the arrays of domains > 1 would beget a name like san-976193852

stableDomain could be used in the pathForDomain, and would be a lot less invasive in terms of changes and maintenance.

[zetlen]

@camsjams That would work too; the worst part about it would be that we would never detect when the requested domains are a strict subset.

If I went:

devcert.certificateFor(['addams.family', 'pugsley.addams.family'])

And then later:

devcert.certificateFor(['pugsley.addams.family'])

It would generate two certificates, when it could easily return the first cert. Fewer certificates is better for a number of reasons, but given the likely patterns of use, this might not be a big deal.

[zetlen]

I should clarify, @camsjams: can you think of a simple way to avoid the issue I mentioned above? If not, then you can make the changes you suggested and I'll merge them, then open the redundancy issue as another ticket.

[camsjams]

As part of the process of converting to a stable hash, it could filter out subset domains.

So a list like:

const domains = [
    'eddie.munster',
    'addams.family',
    'pugsley.addams.family',
    'gomez.addams.family'
];

could be filtered down to :

const domains = [
    'eddie.munster',
    'addams.family'
];

Just for the sake of the hashing of names for folder use.

[zetlen]

That's a great plan. Let's do that.

[camsjams]

Sounds great - working on it and should have by end of week as part of my work stream. Thanks!

[camsjams]

OK @zetlen I've updated to add the features we've discussed. Locally I wrote some unit tests but this repo doesn't have any place for it, so I didn't commit them.

[camsjams]

@zetlen Is this still a feature that is desired? The code is complete and ready for review.

I do see that a recent update has created a merge conflict - I'd be happy to merge master into my code and fix conflict if this feature branch is something that will get used.

[niklasgrahl]

Hi! I was recently looking at using devcert but unfortunately couldn't because I needed multiple SAN. If this is included I'd definitely use it

[ranyitz]

@camsjams This PR looks amazing, I believe that the feature could help many people (me included). @zetlen What else needs to be done? Can I help?

[camsjams]

I had updated the PR per the requested changes, and then master had some updates that created the merge conflict. We've just been using my fork of this for our team:
https://github.com/camsjams/devcert

I am able to remerge this back in if this is still a desired feature.

[amiryonatan]

@camsjams we would appreciate it greatly if you could find the time to remerge it back.
Thanks!

[camsjams]

@amiryonatan Yes I can merge, test, and update this week

[camsjams]

@amiryonatan @zetlen @ranyitz This is remerged - sorry had some personal life things come up, so had to put aside for a bit.

I've remerged and tested, and adjusted my code to work with the updated validation that caused the original merge conflicts.

[camsjams]

If at all useful BTW, you can temporarily clone my repo and

1. npm install
2. npm run build
3. Create a file like below (san.js)
4. Run this file node san.js

const fs = require('fs');
const devcert = require('./dist');


function main(mode = 0) {
    console.log('mode:', mode);
    switch (mode) {
        case 0:
            console.log('generating');
            return devcert.certificateFor([
                'localhost',
                'local.api.example.com',
                'local.cms.example.com',
                'local.example.com'
            ])
                .then(({key, cert}) => {
                    fs.writeFileSync('./tls.key', key);
                    fs.writeFileSync('./tls.cert', cert);
                    return;
                })
                .catch((error) => {
                    console.log('error', error);
                });
        case 1:
            console.log('listing');
            console.log(devcert.configuredDomains());
            break;
        case 2:
            console.log('removing');
            return devcert.removeDomain([
                'localhost',
                'local.api.example.com',
                'local.cms.example.com',
                'local.example.com'
            ]);

    }
}

// update the argument to run each command to confirm that this works
main(0);

[amiryonatan]

@camsjams looks like it works as expected, thank you very much!
@davewasmer @zetlen is there anything else necessary in order to merge this PR onto master?
Thanks!

[amiryonatan]

@Js-Brecht are you able to assist here plz?
all changes were fixed.

[Js-Brecht]

I would love to; this PR looks good. Unfortunately, I don’t have write access. @zetlen is the one to ask. I guess he must be pretty busy if he hasn’t noticed the recent pings yet.

[sghoweri]

+1. I'd really love to see this ship!

[amiryonatan]

@Js-Brecht are you able to assist please?

[Js-Brecht]

@amiryonatan look up a couple comments ☝️

[amiryonatan]

my bad, wanted to tag @zetlen.
@zetlen will you please look into this?