Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add ability to specify endpoint key #176

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Add ability to specify endpoint key #176

wants to merge 1 commit into from

Conversation

iartem
Copy link
Contributor

@iartem iartem commented Feb 8, 2016

Spec says there can be only one connection per host/port, but there is a case with Apple Push Notifications Service where one host/port can be used with multiple certificates. This pull request makes it possible.

Not sure whether this should be merged, but I think it won't hurt.

@nwgh
Copy link
Collaborator

nwgh commented Mar 15, 2016

Sorry for the lag here (the downside of having little time to work on this). Are you talking about one physical host having SNI or Subject Alternate Names in the cert, making that physical host authoritative for multiple origins, and thus coalescing multiple domains onto one connection? (For example, host a.foo.com presents a cert for a.foo.com, b.foo.com, and foobar.com, so all requests for any of those hosts could go on the same connection.)

If so, the coalescing problem is more subtle than just allowing a user-defined key for connection pooling, and will take more than just the change here to get it right (we would want to make sure, among other things, that DNS and SNI match).

If you're meaning something else, could you perhaps explain the situation more in-depth so I can understand what's being fixed before accepting the PR? Thanks.

@iartem
Copy link
Contributor Author

iartem commented Mar 16, 2016

Sure. I used this module to send push notifications through relatively new APN http/2 interface.

Authentication for APN is made via use of client-side certificates. To send a push notification you need to generate a certificate for your app an use it to connect to a single APN endpoint. Certificates for different apps differ not only by SNI (not sure if there is any), but rather completely. Only issuer is the same - Apple.

But this module uses host/port combination to identify connection. While 100% correct according to http/2 spec, this way prevents using this module for APN when you have multiple apps = multiple certificates for the same host/port.

@nwgh
Copy link
Collaborator

nwgh commented Mar 17, 2016

Ah, I see, this is for client certs, sent to authenticate your client (written using node-http2) to the APNS server.

Given that renegotiation is disallowed for h2, yeah, we should probably use a client cert as part of our key. I still don't think that allowing any random key value to be passed in with the options is the right way to go about it, though. We should (when we create the key) look to see if we have a client cert for the connection (which may need some plumbing of its own), and if so, add it to the key. That way we keep our nice coalescing properties, while allowing using the APNS with multiple apps, and we don't open the key up to just anyone to mess with.

As an aside - I looked at the APNS doc you linked to, and man, apple is doing some icky stuff, trying to add requirements to how the HPACK encoder does its job, and breaking layering by sending back 403s if the TLS cert is invalid (instead of failing negotiation at the TLS layer, or just doing authentication entirely at the HTTP layer). Ugh.

@iartem
Copy link
Contributor Author

iartem commented Mar 17, 2016

Oh, it's even more fun than just 403. Certificates issued for 1 year and when they expire (surprise!) connection is reset. They also have some special relationships with SETTINGS frames, especially max concurrent requests.

Also, renegotiation wouldn't be enough anyway, because 2 connections for different apps should be able to live at the same time. So different keys only.

But yes, you're right, it'd be better to add certificate itself to the key. I guess some hex of RSA public key would be enough. But that would require some node-forge dependency to do certificate parsing, wouldn't it?

@nwgh
Copy link
Collaborator

nwgh commented Mar 17, 2016

I'm not even sure we need the key fingerprint, tbh, probably just the full path to the cert file (or some other bit of identifying information) would work.

I'll have to think on this some more to come up with a good solution.

@singhnitesh
Copy link

what if we use different agents, configure them with pfx & passphrase for the respective apps, and pass those as options in http2.request method? IMHO its a much cleaner way than adding support for specifying custom endpoint keys and would not require any change in this library.

I have implemented a proof of concept and it seems to work, but because I don't have much experience in backend work so can't think of major downsides to this approach. If you guys can shine some light on this, it would be really helpful.

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