search

jspm / github

Github Location Service
16 stars 43 forks source link

Add passing of TLS/SSL configuration to request from config #75

Closed cehoffman closed 8 years ago

cehoffman commented 8 years ago

I had a desire to work with a custom CA using a method other than completely turning off SSL verification. The implementation for this option was mentioned in jspm/jspm-cli#980. Since an Enterprise GitHub instance was the only one had a need for currently, that is where I started. I expect in the near future I'll have a need for an NPM registry as well.

I included expand-title because my custom certificates are stored with my dotfiles which are all relative to my home directory. If you would rather go to requiring explicit paths, I'd be willing to change.

cehoffman commented 8 years ago

@guybedford there anything you'd like to see changed about this?

guybedford commented 8 years ago

This looks really great, thanks so much for working on it and sharing.

We can in due course make these a standard global config option just like strictSSL is in https://github.com/jspm/jspm-cli/blob/master/lib/registry.js#L62.

I'm wondering if we should key all these options under a single option - agent or something similar?

The above would allow configuration globally via jspm config agent.ca filename.

cehoffman commented 8 years ago

I went a bit further after this discussion and realized I didn't need to also set the GIT environment variables from my terminal. We could do that here using the information from the configuration object. From there I knew the other request options like passphrase and pfx didn't make any sense because git doesn't support those options.

There is also some additional information in 6a92dbd399eceab19356cd2bce66ad423230698f about how to make this all work on OS X since git with client SSL certificates has a stumbling block that other platforms don't have.

guybedford commented 8 years ago

This is looking really great!

cehoffman commented 8 years ago

Sorry for any snark in a previous comment. I hope everything is shaping up now.

guybedford commented 8 years ago

No worries at all, thanks again for your work on this.

guybedford commented 8 years ago

@cehoffman I think we may have had our first bug on this from https://gitter.im/jspm/jspm?at=56361753e87b056a49cdba4c. Any ideas?

cehoffman commented 8 years ago

It looks like he or msysgit messed up his git config specifying the certificate bundle path incorrectly, I.e. using forward slashes as path separators on Windows.

guybedford commented 8 years ago

Would that error not throw on this line then - https://github.com/jspm/github/pull/75/files#diff-94dc46cf0d324912ab3cf7bed36dbcbbR136?

cehoffman commented 8 years ago

I could see expandTilde maybe doing it, but it has Windows support so I doubt (unverified) it constructs an invalid path using forward slashes on Windows. In the gitter discussion the guy says it was an error with the fresh git install having a bad path for the bundle.

Anyone having an error from this PR currently has to already be messing with their jspm config adding a ca property. Nothing should be changing other than git automatically inheriting the strictSSL configuration which is more permissive. I also should add I have my git using a customized ca bundle in the global config with this change overriding the CA on a single jspm github registry. It is working together well.

cehoffman commented 8 years ago

Oh, perhaps you were thinking he was specifying a CA in the JSPM config? His error was from a CA specified in the global git config.

guybedford commented 8 years ago

@cehoffman thanks for getting back on this so quickly. I also noticed a strange error, and adding https://github.com/jspm/github/commit/7b2f3f808978daaa30e4aacee30703d94a529978 seemed to resolve it here.

cehoffman commented 8 years ago

What was the error? I don't anticipate the change you did to have any problems, it might even be better to reflect the environment.

guybedford commented 8 years ago

@cehoffman it was causing a critical install error in my setup actually. I forget the exact message now, but just wanted to report it may have resolved other things too.