Introduce ranch_transport:peercert/1 callback

[juise]

This callback should be used from cowboy_http and cowboy_http2 instead of a direct call to ssl:peercert/1.

[juise]

@essen could you take a look, please?

[essen]

It looks fine but can you explain the motivation behind this so we can prioritize?

[Maria-12648430]

I wonder, why should this be in ranch_transport? It is a ssl-specific function, so I think it should be in ranch_ssl only instead of requiring all transports to implement it via the behavior.

[juise]

Yeah, the motivation behind that - we are using the TLS, with fast_tls and OpenSSL directly, but not with the erlang SSL module (originally, due to we need the old sslv3, and after that, due to erlang SSL doesn't support TLS 1.0 and TLS 1.1 in the FIPS mode).

I agree, the ssl:peercert is SSL/TLS (sic!) specific function, and it's better to keep it in a specific module like ranch_ssl. But, we have not only ransh_ssl, but also self-made ranch_tls based on ranch_transport behavior. And could return peer cert from fast_tls as well as from erlang ssl. And after that, it's become a transport-specific function. So, based on that, it seems like it's not enough to just to put the ssl:peercert into ranch_ssl, and I believe it's better to put it into ranch_transport.

[Maria-12648430]

So, based on that, it seems like it's not enough to just to put the ssl:peercert into ranch_ssl, and I believe it's better to put it into ranch_transport.

I'm not convinced, or maybe I just don't get it 😅

Having the -callback in ranch_transport will do nothing for you, only require that all modules implementing ranch_transport implement that function, so you still have to implement it in all the custom transport modules you named above, which you can also do without touching the behavior. Having it in there also means that all custom transports out there in the wild will have to be updated to implement it (or to ignore/suppress the warnings) if they want to use the latest Ranch.

But I'm not the one who is calling the shots here, so this is just my two cents 😉 @essen, what do you think?

[juise]

Having the -callback in ranch_transport will do nothing for you, only require that all modules implementing ranch_transport implement that function, so you still have to implement it in all the custom transport modules you named above...

Yes, I understand that, of course, and because of that, I need this fix for this PR - ninenines/cowboy#1559

[juise]

Regarding the

all custom transports out there in the wild will have to be updated to implement it (or to ignore/suppress the warnings) if they want to use the latest Ranch

I absolutely agree with you, and maybe better make such callback an optional. But I don’t do that, because the ranch_tcp already has some callbacks that is not implemented, but throw an error in case they are called. So, it’s possibly make sense make them optional too

[essen]

I think we should implement them and throw an error to make it clearer to the user that the operation is not supported. I'm not against making all 3 callbacks optional for people who write their own transport module but it hasn't come up before.

[Maria-12648430]

I think we should implement them and throw an error to make it clearer to the user that the operation is not supported. I'm not against making all 3 callbacks optional for people who write their own transport module but it hasn't come up before.

So if we add peer_cert to ranch_transport, should we also add other similar functions, like negotiated_protocol for instance?

[essen]

OK clarifying, when I say implement them I mean in ranch_tcp, regardless of the functions being in ranch_transport. So that if the user uses the wrong transport (specifically, ranch_tcp instead of ranch_ssl) the problem is immediately obvious.

As for ranch_transport, we should only add them there if they are set as optional, and otherwise not add them at all.

[juise]

As for ranch_transport, we should only add them there if they are set as

optional, and otherwise not add them at all. But, right now all callbacks in ranch_transport are mandatory, not optional...

[essen]

I'm not sure how that's a problem, we can have mandatory and optional callbacks.

[juise]

I'm a little bit confused, what's the final idea/proposal/solution?

[juhlig]

I'm not against making all 3 callbacks optional for people who write their own transport module but it hasn't come up before.

There are 2 callbacks right now implemented to throw error(not_supported) in ranch_tcp, namely handshake_continue and handshake_cancel. Just my 2ct, but I think that should remain as it is, ie the callbacks should not be made optional. They are used within ranch itself in the handshake phase, it is just that the opportunity to use handshake_continue or handshake_cancel only comes about in a multi-step handshake, if ranch:handshake returns {continue, ...}.

For other functions like peercert, I think they should not be ranch_transport callbacks, optional or not. They can be implemented in and used via ranch_ssl or whatever without there being a callback definition. Another candidate in the same line is negotiated_protocol, btw, so we might add that here while we're at it.

[juise]

I'm definitely don't understand you.

We have the ranch and ranch_transport behavior, this behavior abstracts some common cases of transports/protocol, but also, it could also need some transports/protocol specific. The most important is that behavior said to the implementations what functions and what behavior MUST be implemented. But you said:

For other functions like peercert, I think they should not be ranch_transport callbacks, optional or not. They can be implemented in and used via ranch_ssl or whatever without there being a callback definition. Another candidate in the same line is negotiated_protocol, btw, so we might add that here while we're at it.

In this case, the peercert which needed by ranch is will be called from ranch_transport implementation, but will not be defined in behavior. So, how 3rd party ranch_transport implementation should know that they have to implement and expose such function?

[juhlig]

I'm definitely don't understand you.

I'm sorry =^^= I think I know where the source of the confusion lies, I just can't find a good explanation =^^=

I guess you're thinking of ranch_transport in terms of something like gen_server, but it is different. ranch_transport is just a blueprint for transports, not an abstraction.

(@Maria-12648430, you're better at explaining stuff than me, help us already XD)

[essen]

In short, it's not super important, don't worry about it.

[juise]

Is any chance this PR will be merged, I wouldn't like to fork cowboy and ranch, to workaround the following error:

    exception error: no function clause matching
                     ssl:peercert({tlssock,#Port<0.25>,
                                      #Ref<0.386484451.1934753793.67533>}) (ssl.erl, line 966)
      in function  cowboy_http:init/6 (/home/app/_build/default/lib/cowboy/src/cowboy_http.erl, line 164)

[Maria-12648430]

@juise I may be mistaken, but it seems to me that that error has nothing to do with the suggested change to ranch_transport, or ranch_ssl for that matter... ranch_ssl would do nothing else than call ssl:peercert with the same argument as the direct call does now. Am I missing something? 🤔

[juhlig]

@juise @Maria-12648430 yeah, I wonder, too. Having peercert in the ranch transports would not solve anything there as far as I can tell.

Anyway, @essen may want to merge #338 first, so the tests will pass again.

And @juise, even if this gets merged now, it requires another PR at cowboy to be accepted to use this, and that cowboy version would not work together with older ranch versions where the peercert function is missing. Not sure @essen is eager to go down that road 😶

[juhlig]

@juise oh wait, I remember... You had some sort of custom transport 😅

[essen]

There's some flaky tests it seems but master is using Actions now, so please rebase so your PR can go through CI. Thanks!