Feature: Allow multiple Subject Alternative Name (SAN) extensions #52

camsjams · 2020-03-19T21:06:59Z

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 · 2020-03-19T21:32:25Z

src/constants.ts

@@ -16,29 +16,43 @@ export const configDir = applicationConfigPath('devcert');
 export const configPath: (...pathSegments: string[]) => string = path.join.bind(path, configDir);

 export const domainsDir = configPath('domains');
-export const pathForDomain: (domain: string, ...pathSegments: string[]) => string = path.join.bind(path, domainsDir)
+export const pathForDomain: (domain: string | string[], ...pathSegments: string[]) => string = path.join.bind(path, domainsDir)


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)

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?

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 · 2020-03-20T23:51:30Z

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 · 2020-03-22T00:30:18Z

@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 · 2020-03-22T23:13:39Z

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 · 2020-03-22T23:50:11Z

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 · 2020-03-23T17:55:38Z

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

camsjams · 2020-03-25T21:38:34Z

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

camsjams · 2020-03-31T19:51:06Z

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 · 2020-07-22T22:45:20Z

@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 · 2020-08-03T11:14:05Z

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 · 2021-02-01T10:54:46Z

@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 · 2021-02-01T17:25:09Z

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 · 2021-02-04T08:44:22Z

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

camsjams · 2021-02-07T05:04:24Z

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

camsjams · 2021-02-22T21:11:31Z

@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 · 2021-02-22T23:38:01Z

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

npm install
npm run build
Create a file like below (san.js)
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 · 2021-03-14T11:54:53Z

@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 · 2021-03-21T19:25:16Z

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

Js-Brecht · 2021-03-21T20:13:19Z

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 · 2021-04-02T16:30:17Z

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

amiryonatan · 2021-05-20T08:52:55Z

@Js-Brecht are you able to assist please?

Js-Brecht · 2021-05-20T08:54:35Z

@amiryonatan look up a couple comments ☝️

amiryonatan · 2021-05-20T08:56:22Z

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

Cameron Manavian added 7 commits March 19, 2020 12:37

Feature: Adding multiple domains using subject alternative name (SAN)

2c38c16

Update README with Docker and SAN instructions

f846a72

Update readme

11ad908

Readme polish

9bc69b7

Tweaks

d1f0045

Formatting fixes

38db467

Adjust pathForDomain

ff5173c

camsjams mentioned this pull request Mar 19, 2020

feat(subject_alt_names): Allow multiple subject_alt_names #34

Open

Js-Brecht suggested changes Mar 19, 2020

View reviewed changes

zetlen self-requested a review March 20, 2020 20:56

zetlen requested changes Mar 20, 2020

View reviewed changes

Create a stable hash for directory path of SAN listed domains

678ac28

Cameron Manavian added 2 commits February 22, 2021 12:07

Merge branch 'master' of https://github.com/davewasmer/devcert

d5f7392

Merge SAN with validation features

e94fcc0

Tweak for latest typescript

2c94257

zetlen merged commit be273aa into davewasmer:master Jul 14, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature: Allow multiple Subject Alternative Name (SAN) extensions #52

Feature: Allow multiple Subject Alternative Name (SAN) extensions #52

camsjams commented Mar 19, 2020

Js-Brecht left a comment

Js-Brecht Mar 19, 2020

Js-Brecht Mar 19, 2020 •

edited

camsjams Mar 19, 2020

zetlen left a comment

camsjams commented Mar 20, 2020

zetlen commented Mar 22, 2020

zetlen commented Mar 22, 2020

camsjams commented Mar 22, 2020

zetlen commented Mar 23, 2020

camsjams commented Mar 25, 2020

camsjams commented Mar 31, 2020

camsjams commented Jul 22, 2020 •

edited

niklasgrahl commented Aug 3, 2020

ranyitz commented Feb 1, 2021

camsjams commented Feb 1, 2021

amiryonatan commented Feb 4, 2021

camsjams commented Feb 7, 2021

camsjams commented Feb 22, 2021

camsjams commented Feb 22, 2021

amiryonatan commented Mar 14, 2021

amiryonatan commented Mar 21, 2021

Js-Brecht commented Mar 21, 2021

sghoweri commented Apr 2, 2021

amiryonatan commented May 20, 2021

Js-Brecht commented May 20, 2021

amiryonatan commented May 20, 2021

Feature: Allow multiple Subject Alternative Name (SAN) extensions #52

Feature: Allow multiple Subject Alternative Name (SAN) extensions #52

Conversation

camsjams commented Mar 19, 2020

Js-Brecht left a comment

Choose a reason for hiding this comment

Js-Brecht Mar 19, 2020

Choose a reason for hiding this comment

Js-Brecht Mar 19, 2020 • edited

Choose a reason for hiding this comment

camsjams Mar 19, 2020

Choose a reason for hiding this comment

zetlen left a comment

Choose a reason for hiding this comment

camsjams commented Mar 20, 2020

zetlen commented Mar 22, 2020

zetlen commented Mar 22, 2020

camsjams commented Mar 22, 2020

zetlen commented Mar 23, 2020

camsjams commented Mar 25, 2020

camsjams commented Mar 31, 2020

camsjams commented Jul 22, 2020 • edited

niklasgrahl commented Aug 3, 2020

ranyitz commented Feb 1, 2021

camsjams commented Feb 1, 2021

amiryonatan commented Feb 4, 2021

camsjams commented Feb 7, 2021

camsjams commented Feb 22, 2021

camsjams commented Feb 22, 2021

amiryonatan commented Mar 14, 2021

amiryonatan commented Mar 21, 2021

Js-Brecht commented Mar 21, 2021

sghoweri commented Apr 2, 2021

amiryonatan commented May 20, 2021

Js-Brecht commented May 20, 2021

amiryonatan commented May 20, 2021

Js-Brecht Mar 19, 2020 •

edited

camsjams commented Jul 22, 2020 •

edited