electron-userland / electron-windows-store

:package: Turn Electron Apps into Windows AppX Packages
MIT License
678 stars 86 forks source link

Allow custom distinguished name for `makeCert` #69

Closed jacobq closed 7 years ago

jacobq commented 7 years ago

The goal of this PR is to make it easier to reuse the certificate creation functions in downstream projects by:

  1. Exporting a isValidPublisher function that checks whether or not a string matches the syntax required for the publisher argument passed to makecert.exe (an X.500 distinguished name)
  2. Allow a distinguished name to be passed in to makeCert (won't prepend CN= to publisherName if it's already a valid distinguished name)

An example use case can be seen here https://github.com/electron-userland/electron-forge/pull/161

jacobq commented 7 years ago

@felixrieseberg What are your thoughts regarding this?

felixrieseberg commented 7 years ago

Code looks good to me - real quick though regarding the API: I couldn't really see the breaking change. It looks like current users currently passing in the name without a CN= are still fine, right?

jacobq commented 7 years ago

The change that I was concerned about was the function signature:

function makeCert (publisherName, certFilePath, program)

vs.

function makeCert (parameters)

So, yes, as long as users aren't calling makeCert directly everything should be fine, but if they are then they have to reformat the arguments they're passing. Since makeCert is exported I assume it is considered part of the API, so I wanted to be cautious about changing it. If you're OK with this then I'll add another commit that adjusts any internal calls (e.g. in setup.js) and any existing documentation I can find to use the updated format then remove the [WIP] tag here when I think it can be safely merged.

felixrieseberg commented 7 years ago

We can just not break the api - let's do a quick check on the signature of the call and quickly turn it into parameters. That way we can change the API, but we don't break existing use cases.

I can add that real quick - or you can just add it to the PR.

jacobq commented 7 years ago

Sure, if that is your preference we can do that too. Take a look at a4e61da and see if it's close to what you had in mind.

jacobq commented 7 years ago

I just also noticed that there is at least one npm module for DN parsing / validating / escaping, so I'm going to look around a little bit and see if I can modify some of the code added in this PR to use a module like that instead. This could also help with #70

jacobq commented 7 years ago

@felixrieseberg makecert.exe doesn't seem to exactly follow any published standard I could find, so I built-up a regular expression in isValidPublisherName that matches the actual behavior as best I can tell. This should be ready to merge now, but if you've got time to look it over once more (I assume you're at EmberConf :hamster: right now) I'd appreciate it.

felixrieseberg commented 7 years ago

Perfect, that's exactly what I had in mind. Thank you so much!

jacobq commented 7 years ago

No problem. I'm working on some changes to how appmanifest.xml gets written to address #70, so be on the lookout for another PR soon. After that, if you're comfortable cutting a release then I can finally close out the downstream electron-forge PR by having it use your makeCert function instead of its own.