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

Introduce ranch_transport:peercert/1 callback #337

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

Conversation

juise
Copy link

@juise juise commented Jan 7, 2022

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

@juise
Copy link
Author

juise commented Jan 12, 2022

@essen could you take a look, please?

@essen
Copy link
Member

essen commented Jan 12, 2022

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

@Maria-12648430
Copy link
Contributor

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
Copy link
Author

juise commented Jan 13, 2022

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
Copy link
Contributor

Maria-12648430 commented Jan 13, 2022

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
Copy link
Author

juise commented Jan 13, 2022

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
Copy link
Author

juise commented Jan 13, 2022

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
Copy link
Member

essen commented Jan 15, 2022

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
Copy link
Contributor

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
Copy link
Member

essen commented Jan 18, 2022

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
Copy link
Author

juise commented Jan 18, 2022 via email

@essen
Copy link
Member

essen commented Jan 18, 2022

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

@juise
Copy link
Author

juise commented Jan 19, 2022

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

@juhlig
Copy link
Contributor

juhlig commented Jan 20, 2022

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
Copy link
Author

juise commented Jan 24, 2022

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
Copy link
Contributor

juhlig commented Jan 24, 2022

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
Copy link
Member

essen commented Jan 24, 2022

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

@juise
Copy link
Author

juise commented Feb 15, 2022

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
Copy link
Contributor

@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
Copy link
Contributor

juhlig commented Feb 15, 2022

@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
Copy link
Contributor

juhlig commented Feb 15, 2022

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

@essen essen added this to the 2.2 milestone Nov 23, 2023
@essen
Copy link
Member

essen commented Dec 5, 2023

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

@essen essen removed this from the 2.2 milestone Jan 9, 2024
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.

4 participants