Skip to content

add missing verify_fun mfa translation #9692

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

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

Conversation

danj3
Copy link

@danj3 danj3 commented Apr 4, 2025

inet_tls_dist is missing a configuration translation for verify_fun from an mfa tuple to a closure that can be called during ssl_handshake.

@IngelaAndin This is the missing verify_fun translation I spoke of, applied to master

@CLAassistant
Copy link

CLAassistant commented Apr 4, 2025

CLA assistant check
All committers have signed the CLA.

Copy link
Contributor

github-actions bot commented Apr 4, 2025

CT Test Results

    2 files     66 suites   25m 57s ⏱️
  815 tests   772 ✅  43 💤 0 ❌
4 180 runs  3 380 ✅ 800 💤 0 ❌

Results for commit bad7d90.

♻️ This comment has been updated with latest results.

To speed up review, make sure that you have read Contributing to Erlang/OTP and that all checks pass.

See the TESTING and DEVELOPMENT HowTo guides for details about how to run test locally.

Artifacts

// Erlang/OTP Github Action Bot

@IngelaAndin IngelaAndin added the team:PS Assigned to OTP team PS label Apr 7, 2025
@RaimoNiskanen
Copy link
Contributor

Sorry, I don't understand what this change enables that cannot be done already.

The existing {verify_fun, {Fun, Init}} should cover everything that {verify_fun, {Mod,Func,Init}} could do, or what am I missing?

@danj3
Copy link
Author

danj3 commented Apr 10, 2025

Ah I forgot something. This change is to support -ssl_dist_optfile that looks like:

[{server,[{fail_if_no_peer_cert,true},
          {certfile,"erldist_cert.pem"},
          {keyfile,"erldist_privkey.pem"},
          {cacertfile,"erldist_cert.pem"},
          {verify,verify_peer},
          {verify_fun,{localca,verify,"erldist_cert.pem"}}]},
 {client,[{certfile,"erldist_cert.pem"},
          {keyfile,"erldist_privkey.pem"},
          {cacertfile,"erldist_cert.pem"},
          {verify,verify_peer},
          {verify_fun,{localca,verify,"erldist_cert.pem"}}]}].

These quasi-mfa verify_fun tuples are turned into the closure functions by this patch. The existing code was ignoring/passing over the verify_fun of this form. This form is based on what I got out of the Erlang Distribution over TLS

@IngelaAndin
Copy link
Contributor

IngelaAndin commented Apr 11, 2025

@danj3 I do not think you should base your new code on how legacy option handling was done. The latest version of the documentation that you are referring to, of erlang distribution, clearly states:
"The following section describes TLS Option handling prior to OTP 20.2 and can only handle a small subset of the actual available options. It is here only for the sake of backwards compatibility ." And I would claim that it is an error in the documentation because verify_fun has always been {verify_fun, {fun(), UserState::term()}}

@RaimoNiskanen
Copy link
Contributor

@IngelaAndin wrote:

And I would claim that it is an error in the documentation because verify_fun has always been {verify_fun, {fun(), UserState::term()}}

I'd say that the documentaion is correct. There was/is a conversion for a command line option -ssl_dist_opt client_verify_fun '{localca,verify,"erldist_cert.pem"}' that converts it into {verify_fun, {fun localca:verify/3,"erldist_cert.pem"}} for the client. And the corresponding for the server.

It is a conversion done for the clumsy legacy command line options, not needed from an -ssl_dist_optfile.

@IngelaAndin
Copy link
Contributor

It is a conversion done for the clumsy legacy command line options, not needed from an -ssl_dist_optfile.

Well, even if the doc is correct why do new things that takes in consideration old legacy thing that we do not recommend using.

@RaimoNiskanen
Copy link
Contributor

I just pointed out that the documentation is not incorrect.

I see no point in adding support for this legacy format to -ssl_dist_optfile, and did not argue for that.

@danj3
Copy link
Author

danj3 commented Apr 14, 2025

I agree that supporting the legacy verify_fun option in the optfile is pointless. I added an ssl doc section and reverted the code change in the #9691 the pr to make very clear the specific use of verify_fun in the optfile. While the example is implied by the existing documentation, a concrete example of this kind would have saved me considerable time. I can update this PR as well at your request, as well as any content changes to the doc.

@IngelaAndin IngelaAndin added the waiting waiting for changes/input from author label May 6, 2025
@IngelaAndin IngelaAndin added this to the OTP-28.1 milestone May 6, 2025
@IngelaAndin IngelaAndin changed the base branch from master to maint May 27, 2025 09:24
inet_tls_dist is missing a configuration translation for
verify_fun from an mfa tuple to a closure that can be called during
ssl_handshake.
@IngelaAndin IngelaAndin force-pushed the dj/master/dist_verify_fun branch from 0573dc6 to bad7d90 Compare May 27, 2025 14:30
@IngelaAndin
Copy link
Contributor

Rebased it on OTP-28

@IngelaAndin IngelaAndin removed the waiting waiting for changes/input from author label May 27, 2025
Copy link
Contributor

@RaimoNiskanen RaimoNiskanen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From what I understand this should be changed into a documentation update just like PR #9691

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
team:PS Assigned to OTP team PS
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants