davewasmer / devcert

Local HTTPS development made easy
1.28k stars 75 forks source link

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

Closed camsjams closed 3 years ago

camsjams commented 4 years ago

Revision of https://github.com/davewasmer/devcert/pull/34 and fixes https://github.com/davewasmer/devcert/issues/33

Changes:

camsjams commented 4 years ago

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 commented 4 years ago

@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 commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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

camsjams commented 4 years ago

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

camsjams commented 4 years ago

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 commented 4 years ago

@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 commented 4 years ago

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 commented 3 years ago

@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 commented 3 years ago

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 commented 3 years ago

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

camsjams commented 3 years ago

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

camsjams commented 3 years ago

@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 commented 3 years ago

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 commented 3 years ago

@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 commented 3 years ago

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

Js-Brecht commented 3 years ago

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 commented 3 years ago

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

amiryonatan commented 3 years ago

@Js-Brecht are you able to assist please?

Js-Brecht commented 3 years ago

@amiryonatan look up a couple comments ☝️

amiryonatan commented 3 years ago

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