Skip to content
This repository was archived by the owner on Mar 10, 2026. It is now read-only.

added option to request, require and verify client certificates#48

Open
SierraGolf wants to merge 1 commit into
devsu:masterfrom
SierraGolf:master
Open

added option to request, require and verify client certificates#48
SierraGolf wants to merge 1 commit into
devsu:masterfrom
SierraGolf:master

Conversation

@SierraGolf

Copy link
Copy Markdown

currently the framework does not support the 3rd parameter checkClientCertificate of the grpc.ServerCredentials.createSsl function. this pull request adds support for it.

@coveralls

coveralls commented Sep 16, 2017

Copy link
Copy Markdown

Coverage Status

Coverage remained the same at 100.0% when pulling 7d27b07 on SierraGolf:master into a1ea74b on devsu:master.

@SierraGolf

Copy link
Copy Markdown
Author

@c3s4r can you have a look at this?

@c3s4r

c3s4r commented Sep 27, 2017

Copy link
Copy Markdown
Contributor

Hi @SierraGolf, thanks for the PR. I haven't had a chance to review it (on vacations now), but I'll take a look early next week.

@c3s4r

c3s4r commented Oct 2, 2017

Copy link
Copy Markdown
Contributor

Hi @SierraGolf, the PR looks good, the only missing piece is a test with checkClientCert = true. Please add it if possible, and I will merge the change. Otherwise, I will do it but not now, since I'll be busy on other urgent projects. Thanks!

@SierraGolf

Copy link
Copy Markdown
Author

I think a test similar to this one would make sense:
https://github.com/SierraGolf/condor-framework/blob/7d27b07f5294984490b0a134c212b1b00f06fced/lib/server.spec.js#L230

However, the test currently cannot assert on the properties of the credentials object, so we would need to spy on this somehow. Not sure what is the best approach to do that.

@c3s4r

c3s4r commented Oct 5, 2017

Copy link
Copy Markdown
Contributor

Ok, thanks. I'll take a look hopefully during next week.

@SierraGolf

Copy link
Copy Markdown
Author

any chance this will get merged and released?

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants