feat(subject_alt_names): Allow multiple subject_alt_names #34

jamsea · 2019-06-15T00:05:17Z

Related to: #33 allows a user to pass in multiple domain names for one certificate without introducing breaking API changes.

Js-Brecht · 2019-10-15T18:54:33Z

This would be a welcome addition. Might also be a good idea to allow IP addresses

chimurai · 2019-07-03T21:56:06Z

yarn.lock

-  resolved "https://registry.yarnpkg.com/tslib/-/tslib-1.8.1.tgz#6946af2d1d651a7b1863b531d6e5afa41aa44eac"
+tslib@^1.10.0:
+  version "1.10.0"
+  resolved "https://system1.jfrog.io/system1/api/npm/npm-virtual/tslib/-/tslib-1.10.0.tgz#c3c19f95973fb0a62973fb09d90d961ee43e5c8a"


Think you're accidentally pointing to your private registry?

Nice catch @chimurai

zetlen

update: i actually have some more concerns about this, see later review

~~@jamsea This is a really useful addition, so I'm gonna update these to fix them and then merge it.~~

Fair warning: I'm gonna try to back out your dependency updates completely, since I'm not sure they are strictly necessary for this PR, and they should maybe come in a separate one. I'll do a full regression.

zetlen · 2019-11-21T16:41:19Z

yarn.lock

-  resolved "https://registry.yarnpkg.com/tslib/-/tslib-1.8.1.tgz#6946af2d1d651a7b1863b531d6e5afa41aa44eac"
+tslib@^1.10.0:
+  version "1.10.0"
+  resolved "https://system1.jfrog.io/system1/api/npm/npm-virtual/tslib/-/tslib-1.10.0.tgz#c3c19f95973fb0a62973fb09d90d961ee43e5c8a"


Nice catch @chimurai

Re-reviewing

zetlen

Thanks again for the submission; this is really useful. I have some recommended code changes as well as a couple questions.

zetlen · 2019-11-21T17:16:52Z

package.json

@@ -27,7 +27,7 @@
  "homepage": "https://github.com/davewasmer/devcert#readme",
  "devDependencies": {
    "standard-version": "^4.3.0",
-    "typescript": "^2.7.0"
+    "typescript": "^2.9.2"


Did you find that you needed to update typescript and tslib for your changes to work? If not, I'd like to save dep updates for separate PRs.

@zetlen yes, at the time I needed to update typescript for flatMap to work

zetlen · 2019-11-21T17:18:02Z

tsconfig.json

@@ -2,6 +2,9 @@
  "compileOnSave": false,
  "compilerOptions": {
      "declaration": true,
+      "lib": [


Same question as the dependency updates: is it possible to break this into another PR?

zetlen · 2019-11-21T17:26:52Z

src/index.ts

-export async function certificateFor(domain: string, options: Options = {}) {
-  debug(`Certificate requested for ${ domain }. Skipping certutil install: ${ Boolean(options.skipCertutilInstall) }. Skipping hosts file: ${ Boolean(options.skipHostsFile) }`);
+export async function certificateFor(domains: string | string[], options: Options = {}) {
+  const domain = Array.isArray(domains) ? domains[0] : domains;


I appreciate the backwards-compatibility efforts here. I recommend that you simplify the following code by making domains always an array and then using domains only for the rest of the code. Then you can remove the conditionals for when it's one domain.

-export async function certificateFor(domains: string | string[], options: Options = {}) { - const domain = Array.isArray(domains) ? domains[0] : domains; +export async function certificateFor(requestedDomains: string | string[], options: Options = {}) { + const domains = Array.isArray(requestedDomains) ? requestedDomains : [requestedDomains]

If you do that, you can rename all the other private identifiers to plurals so the code is easier to read, so pathForDomain becomes pathForDomains et cetera. This makes the types easier to follow!!

Cool @zetlen , I thought that too I just wanted to save the backward compatibility. If we're not bothered about backward compatibility (it's really not that big of a deal if we do a major version bump) I'll change this

zetlen · 2019-11-21T17:28:53Z

src/certificates.ts

@@ -15,7 +15,8 @@ const debug = createDebug('devcert:certificates');
 * individual domain certificates are signed by the devcert root CA (which was
 * added to the OS/browser trust stores), they are trusted.
 */
-export default async function generateDomainCertificate(domain: string): Promise<void> {
+export default async function generateDomainCertificate(domains: string | string[]): Promise<void> {
+  const domain = Array.isArray(domains) ? domains[0] : domains;


This is an example of the kind of code you can simplify if you cast a request for a single domain into an array of length 1 earlier in the process.

camsjams · 2020-03-17T19:55:10Z

What happened to this PR? Need help?

camsjams · 2020-03-17T20:10:41Z

tsconfig.json

@@ -2,6 +2,9 @@
  "compileOnSave": false,
  "compilerOptions": {
      "declaration": true,
+      "lib": [
+        "esnext"
+      ],


While running the PR branch, it seems as though this pull request requires upgrades to Node environment, with the usage of functions like flatMap requiring Node 11+ unless this line is removed from tsconfig

@camsjams if it were me, I'd forego the use of flatMap() altogether. A single reduce() (as opposed to flatMap().map()) would solve the problem, I think, and would be better supported.

Yup, I upgraded it because of flatMap at the time.

Js-Brecht · 2020-03-17T21:32:33Z

@camsjams this project is sporadically maintained. I'm not even sure the author of this PR is even getting notifications

I guess we can try pinging @jamsea 🤷‍♂️.

If that doesn't work, maybe it would be worthwhile to fork the changes, submit a new PR, and ping zetlen.

jamsea · 2020-03-17T21:37:48Z

@Js-Brecht sorry! All my notifications were going through an email filter 🤦‍♂ I just noticed now because I saw a push notification on my phone. I'll take a read now

jamsea · 2020-03-17T21:42:37Z

@camsjams if you're free this week and want to take this over that'd be helpful. I'm on another project now so I wouldn't be able to get this until next week. I'll fix notifications so I'm available though

camsjams · 2020-03-17T22:25:23Z

I can take this on, I will probably submit a new PR with the suggestions applied and more minimal changes. I was successfully able to use this feature add to create multiple alt names locally so I am happy to get this merged into the main branch.

zetlen · 2020-03-19T15:44:50Z

Maintainer here. I prefer to call it "periodically maintained". Most of the codebase is mature; additional fun stuff would require a big refactor, which I'd probably put in a new library.

But even bugfixes require a pretty big regression, using multiple VMs; a lot of the code directly touches OS features, so the only useful testing is end-to-end. So that slows down the processing of PRs somewhat.

Thanks very much in advance for your help, @camsjams. As soon as you've updated this, I can get it merged fast.

Js-Brecht · 2020-03-19T20:25:46Z

I prefer to call it "periodically maintained".

No offense meant, @zetlen. You guys have done a great job on this project, and the fact that it needs little maintenance is an excellent indicator of that.

camsjams · 2020-03-19T21:09:22Z

Here is the new #52, we can probably close this current PR in favor of #52 if everyone here agrees.

Basically the same changes that @jamsea had made with the code review comments from @zetlen

camsjams · 2021-02-22T23:33:49Z

Let's close this one as I have branched from @jamsea code (thanks!) and this PR has been remerged again with Master following some code review.

See new PR here: #52

evsar3 · 2023-01-29T19:17:51Z

...Might also be a good idea to allow IP addresses

I've have submitted a new PR (#91) with exactly this feature. I hope the maintainer could merge it someday... 🤞

Bumps [@types/configstore](https://github.com/DefinitelyTyped/DefinitelyTyped/tree/HEAD/types/configstore) from 2.1.1 to 4.0.0. - [Release notes](https://github.com/DefinitelyTyped/DefinitelyTyped/releases) - [Commits](https://github.com/DefinitelyTyped/DefinitelyTyped/commits/HEAD/types/configstore) Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

James Hush added 7 commits June 14, 2019 17:04

feat(subject_alt_names): Allow multiple subject_alt_names

8463211

Change alt names

1e7527c

Add flatMap

db54c8a

Fix typescript errors, add flatMap support in typescript

f8c5bae

Switch public api

c06e68f

Fix the domain vs domains change

a50d7b9

Fix typescript stuff

1c77d6c

chimurai reviewed Oct 16, 2019

View reviewed changes

zetlen previously requested changes Nov 21, 2019

View reviewed changes

Try this?

08038ba

zetlen force-pushed the multiple-alt-names branch from 95f2bd5 to 08038ba Compare November 21, 2019 17:18

zetlen requested changes Nov 21, 2019

View reviewed changes

camsjams reviewed Mar 17, 2020

View reviewed changes

camsjams mentioned this pull request Mar 19, 2020

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

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(subject_alt_names): Allow multiple subject_alt_names #34

feat(subject_alt_names): Allow multiple subject_alt_names #34

jamsea commented Jun 15, 2019

Js-Brecht commented Oct 15, 2019

chimurai Jul 3, 2019

zetlen Nov 21, 2019

zetlen left a comment •

edited

zetlen Nov 21, 2019

zetlen left a comment

zetlen Nov 21, 2019

jamsea Mar 17, 2020

zetlen Nov 21, 2019

zetlen Nov 21, 2019

jamsea Mar 17, 2020 •

edited

zetlen Nov 21, 2019

jamsea Mar 17, 2020

camsjams commented Mar 17, 2020

camsjams Mar 17, 2020

Js-Brecht Mar 17, 2020

jamsea Mar 17, 2020

Js-Brecht commented Mar 17, 2020

jamsea commented Mar 17, 2020

jamsea commented Mar 17, 2020

camsjams commented Mar 17, 2020

zetlen commented Mar 19, 2020 •

edited

Js-Brecht commented Mar 19, 2020

camsjams commented Mar 19, 2020

camsjams commented Feb 22, 2021

evsar3 commented Jan 29, 2023

feat(subject_alt_names): Allow multiple subject_alt_names #34

Are you sure you want to change the base?

feat(subject_alt_names): Allow multiple subject_alt_names #34

Conversation

jamsea commented Jun 15, 2019

Js-Brecht commented Oct 15, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zetlen left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zetlen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jamsea Mar 17, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

camsjams commented Mar 17, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Js-Brecht commented Mar 17, 2020

jamsea commented Mar 17, 2020

jamsea commented Mar 17, 2020

camsjams commented Mar 17, 2020

zetlen commented Mar 19, 2020 • edited

Js-Brecht commented Mar 19, 2020

camsjams commented Mar 19, 2020

camsjams commented Feb 22, 2021

evsar3 commented Jan 29, 2023

zetlen left a comment •

edited

jamsea Mar 17, 2020 •

edited

zetlen commented Mar 19, 2020 •

edited