Closed davidgtonge closed 6 years ago
include self_signed too please. While they’re gonna use the same switch case It’d be good to have em both mentioned
OK - it should be just a case of mentioning it on the client side. Implementing it on the provider will be a bit trickier.
Well, yeah. But i’m working on that for oidc-provider already.
Brilliant - looking forward to see it, let me know if I can help at all
A short call to get a second opinion on some topics sometime next week would be great.
Sounds good - pls email me.
I've made an initial commit for this here: https://github.com/panva/node-openid-client/commit/8e4acf8f0d722d490705df70bc1eb18994468cf7
The params: client_tls_key
, client_tls_cert
and client_tls_ca
are made up, so I wanted to get an opinion on them.
Also should we assert anywhere that if tls_client_auth
is selected that the the key and cert must be provided.
The client_tls_ca
option is also interesting. In some ways its unrelated to client authentication, but for the places that I've used tls_client_auth
the token endpoint has been protected by a certificate signed by a private ca, but the discovery endpoint has been protected using normal browser certs. For this reason it would be good to have the option to specify aca
for calling the token endpoint specifically rather than just as a default http option.
Once we agree on the approach I can make sure documentation is added for this feature.
Sounds good - pls email me.
Dont have your contact. Mine is in the github profile, just msg me and we can setup a call.
That would ve OB specific, requiring client cert auth for all calls. Probably we have to make up a profile structure. Not a fan of made up metadata mixed with iana registered ones, rather provide a separte contructor argument. Eg with the profile and then profile specific options. All certs and CAs for different parts of OB, a subset of that for FAPI.
Probably, for OB the client auth can be pushed to defaults, with the ca being pushed to token/intro/revo calls.
But you already need those defaults for discovery then... sigh OB 😂 edit: nwvermind, the default http opts are set on Issuer, not Client. This can work.
So re the TLS set-up, the OB issue may not be that unique. I think there may be other environments where TLS mutual auth is required for the token endpoint and the revocation endpoint (as a form of client authentication), it may be required for the userinfo endpoint (for sender constrained access tokens) and it likely wont be required at the discovery endpoint.
If the only way to configure TLS options is using the defaultHTTPOptions
then it is hard to support these variations. It also feels a bit wrong setting the client authentication keys on the issuer
instance rather than the client
instance.
I agree with you that the params client_tls_key, client_tls_cert and client_tls_ca aren't great. I see a few options:
We allow setting defaultHTTPOptions
on both the Issuer and the Client (those set on the Client will override those set on the issuer. This is quite close to what we have now, it would make it clearer that keys are being set for a client, and it would support the use case of different http options for the discovery endpoint vs the others.
We add some sort of configuration options that allows the implementer to specify http options per endpoint
What do you think? I may be missing something.
@panva which approach do you think we should take?
@davidgtonge still mulling it over...
@davidgtonge I'm going to go ahead and close this for now, mtls is still on my radar and will likely be included in v3.x
As detailed in this draft: https://tools.ietf.org/html/draft-ietf-oauth-mtls-11
Currently this mode can be achieved with
node-openid-client
by setting thetoken_endpoint_auth_method
tonone
and setting theIssuer.defaultHttpOptions
to include the cert, key and ca.However this isn't ideal - it would be great to add
tls_client_auth
as an option.I'll have a go at a PR for this