Skip to content

Adding ability to use PFX files#768

Merged
herom merged 1 commit into
vpulim:masterfrom
tomgco:pfx-patch
Nov 2, 2015
Merged

Adding ability to use PFX files#768
herom merged 1 commit into
vpulim:masterfrom
tomgco:pfx-patch

Conversation

@tomgco

@tomgco tomgco commented Oct 29, 2015

Copy link
Copy Markdown
Contributor

Instead of extending ClientSSLSecurity to handle keys, certs, etc plus pfx keys, I have instead opted to create a new class specifically to use pfx files.

@jsdevel

jsdevel commented Oct 29, 2015

Copy link
Copy Markdown
Collaborator

I like this strategy. Can you add support for passwords? I feel that may need to be an optional 3rd param.

@tomgco

tomgco commented Oct 29, 2015

Copy link
Copy Markdown
Contributor Author

I like this strategy. Can you add support for passwords? I feel that may need to be an optional 3rd param.

Sure thing

@tomgco

tomgco commented Oct 29, 2015

Copy link
Copy Markdown
Contributor Author

Oops, failing for older versions of node, will fix

@tomgco

tomgco commented Oct 29, 2015

Copy link
Copy Markdown
Contributor Author

Ok @jsdevel, I have added in passphrase support for you, tests are also now passing.

Comment thread lib/security/ClientSSLSecurityPFX.js Outdated

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

can you update this jsdoc?

@jsdevel

jsdevel commented Oct 30, 2015

Copy link
Copy Markdown
Collaborator

Pretty close @tomgco ! Once the last minor comments are addressed we'll merge.

@tomgco

tomgco commented Nov 2, 2015

Copy link
Copy Markdown
Contributor Author

Pretty close @tomgco ! Once the last minor comments are addressed we'll merge.

Thanks, just adding these changes now.

@tomgco

tomgco commented Nov 2, 2015

Copy link
Copy Markdown
Contributor Author

Ok, @jsdevel should be all working now and up to scratch.

Want me to rebase my commits into one, or is it good as they are?

@herom

herom commented Nov 2, 2015

Copy link
Copy Markdown
Contributor

Thanks a lot for your time and effort @tomgco 👍

Please rebase and squash your commits into a single one as stated in our Guidelines on Submitting a Pull Request in order to get your PR merged 😃

This adds supports for PFX files as an alternative to cert and keys.

Support for passphrase's
Fixed issues with ClientSSLSecurityPFX in 0.1 and 0.8
Additional documentation and tests
@tomgco

tomgco commented Nov 2, 2015

Copy link
Copy Markdown
Contributor Author

We are all rebased and should be ready to go, thanks @herom and @jsdevel!

@herom

herom commented Nov 2, 2015

Copy link
Copy Markdown
Contributor

Thanks a ton @tomgco - it's people like you who make this library so awesome 😃

herom added a commit that referenced this pull request Nov 2, 2015
Adding ability to use PFX files
@herom
herom merged commit 1ae500e into vpulim:master Nov 2, 2015
@jsdevel

jsdevel commented Nov 2, 2015

Copy link
Copy Markdown
Collaborator

Thanks a ton @tomgco - it's people like you who make this library so awesome 😃

+1

@herom

herom commented Dec 9, 2015

Copy link
Copy Markdown
Contributor

@tomgco Actually tests are failing over at TravisCI because client-password.pfx has expired and therefore the test "ClientSSLSecurityPFX should be usable in a request" (see here) throws an error.

Since you introduced this valuable extension and provided the files for testing would it be possible for you to add a valid file so that the tests are passing? Thanks a lot 👍

jsdevel added a commit that referenced this pull request Dec 17, 2015
@jsdevel

jsdevel commented Dec 17, 2015

Copy link
Copy Markdown
Collaborator

@tomgco can you re-generate the pfx files with a really long expire time or upload the command you used to create them?

@jsdevel

jsdevel commented Dec 17, 2015

Copy link
Copy Markdown
Collaborator

It would be nice if we scripted these with bash or something.

@tomgco

tomgco commented Dec 18, 2015

Copy link
Copy Markdown
Contributor Author

@jsdevel Hi, sorry about this, I haven’t got the scripts on hand at the moment (on different machine) but should be able to upload a script and a new cert come the weekend.

It would be nice if we scripted these with bash or something.

Even better, I have a script on that other machine already in bash, will require openssl though - but thats a given 💃

@herom

herom commented Dec 18, 2015

Copy link
Copy Markdown
Contributor

@tomgco 👍

@jsdevel should we restart all tests on currently open PRs and merge them since you've commented out the currently invalid test?

@jsdevel

jsdevel commented Dec 18, 2015

Copy link
Copy Markdown
Collaborator

should be able to upload a script and a new cert come the weekend.

Thanks @tomgco! That would be great 😄

should we restart all tests on currently open PRs and merge them since you've commented out the currently invalid test?

I think we'll need to call for a rebase first, otherwise the restart will continue to fail with the test uncommented. But yes, we should notify other PR authors.

diarmaidm pushed a commit to diarmaidm/node-soap that referenced this pull request Feb 3, 2016
Adding ability to use PFX files
diarmaidm pushed a commit to diarmaidm/node-soap that referenced this pull request Feb 3, 2016
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.

3 participants