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

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

Merged
merged 11 commits into from
Jul 14, 2021

Conversation

camsjams
Copy link
Contributor

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

Copy link
Contributor

@Js-Brecht Js-Brecht left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

src/constants.ts Outdated
@@ -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)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Contributor

@Js-Brecht Js-Brecht Mar 19, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@zetlen zetlen self-requested a review March 20, 2020 20:56
Copy link
Collaborator

@zetlen zetlen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Contributor Author

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
Copy link
Collaborator

zetlen commented Mar 22, 2020

@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
Copy link
Collaborator

zetlen commented Mar 22, 2020

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
Copy link
Contributor Author

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
Copy link
Collaborator

zetlen commented Mar 23, 2020

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

@camsjams
Copy link
Contributor Author

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

@camsjams
Copy link
Contributor Author

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
Copy link
Contributor Author

camsjams commented Jul 22, 2020

@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
Copy link

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
Copy link

ranyitz commented Feb 1, 2021

@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
Copy link
Contributor Author

camsjams commented Feb 1, 2021

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
Copy link

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

@camsjams
Copy link
Contributor Author

camsjams commented Feb 7, 2021

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

@camsjams
Copy link
Contributor Author

@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
Copy link
Contributor Author

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
Copy link

@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
Copy link

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

@Js-Brecht
Copy link
Contributor

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
Copy link

sghoweri commented Apr 2, 2021

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

@amiryonatan
Copy link

@Js-Brecht are you able to assist please?

@Js-Brecht
Copy link
Contributor

@amiryonatan look up a couple comments ☝️

@amiryonatan
Copy link

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

@zetlen zetlen merged commit be273aa into davewasmer:master Jul 14, 2021
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.

Multiple subject_alt_names
7 participants