-
Notifications
You must be signed in to change notification settings - Fork 186
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
base: master
Are you sure you want to change the base?
Conversation
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. |
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. |
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. |
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? |
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. |
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. |
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.