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

Ingela/ssl/custom sign fun #7898

Merged
merged 4 commits into from
Dec 12, 2023

Conversation

IngelaAndin
Copy link
Contributor

@IngelaAndin IngelaAndin commented Nov 22, 2023

Last commit show what we like the API to look like in #7475 (Commits now squashed and this PR replaces #7475)

Design decisions:

Keep all engine stuff in ssl, although it is a corner case of the more general
customization added, it has nothing to do with the functionality in public_key.

Keep options sign/encrypt_private options explicit in ssl, where the knowledge of correct options belong, do not rely on public_key defaults that are not even documented.

There has to be a legacy fun option for TLS legacy versions using encrypt_private as new version and legacy version can be supported in the same time and is negotiated.

We do not want to support funs on format {Module, Fun}. fun() is sufficient and you can
use fun(X,Y) ... or fun M:F/2

Copy link
Contributor

github-actions bot commented Nov 22, 2023

CT Test Results

       3 files       71 suites   49m 6s ⏱️
   958 tests    925 ✔️   33 💤 0
3 891 runs  3 134 ✔️ 757 💤 0

Results for commit ac3a3b0.

♻️ 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 Nov 22, 2023
@IngelaAndin IngelaAndin self-assigned this Nov 22, 2023
@ziopio
Copy link
Contributor

ziopio commented Nov 22, 2023

true, we can use M:F/2 to set the function in ssl_dist_ops term file instead of {M, F}

@IngelaAndin IngelaAndin force-pushed the ingela/ssl/custom_sign_fun branch 3 times, most recently from a2c6e52 to 39e88d8 Compare November 23, 2023 15:48
@IngelaAndin IngelaAndin added the testing currently being tested, tag is used by OTP internal CI label Nov 23, 2023
lib/ssl/doc/src/ssl.xml Outdated Show resolved Hide resolved
@IngelaAndin IngelaAndin force-pushed the ingela/ssl/custom_sign_fun branch 3 times, most recently from 750ee1f to 2ff6ac4 Compare November 27, 2023 10:09
@IngelaAndin
Copy link
Contributor Author

@ziopio I think we will also want to be able to give custom options to the fun, to for instance be able to provide some kind or reference to the key to enable the fun to find/access it. In the test you wrote this is not necessary as the key is directly accessible in as it only wraps the standard keys.

@IngelaAndin IngelaAndin requested review from dgud and removed request for ziopio November 27, 2023 10:37
@ziopio
Copy link
Contributor

ziopio commented Nov 27, 2023

@IngelaAndin Yeah we do not have an use-case since in our scenario we control both the code that uses SSL and the signfun and a developer can just embed his/her options inside the function implementation. That said such option can help writing a general purpose custom function. So yeah, I think is a good Idea 👍

lib/public_key/doc/src/public_key.xml Show resolved Hide resolved
lib/ssl/test/ssl_cert_tests.erl Outdated Show resolved Hide resolved
@IngelaAndin
Copy link
Contributor Author

Waiting for @dgud review

Copy link
Contributor

@dgud dgud left a comment

Choose a reason for hiding this comment

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

MIssing test in ssl_api_SUITE:options_whitebox().

lib/public_key/doc/src/public_key.xml Outdated Show resolved Hide resolved
lib/public_key/doc/src/public_key.xml Outdated Show resolved Hide resolved
lib/public_key/src/public_key.erl Show resolved Hide resolved
lib/ssl/src/ssl.erl Show resolved Hide resolved
@dgud
Copy link
Contributor

dgud commented Dec 5, 2023

Also a lot of white-space changes in ssl.erl that is touching (not emacs?) non updated code which will cause a lot of merge-conflicts later.

@IngelaAndin
Copy link
Contributor Author

@dgud please rereview doc

lib/ssl/doc/src/ssl.xml Outdated Show resolved Hide resolved
ziopio and others added 4 commits December 8, 2023 16:26
This option allows the user to define a function tasked to sign an ssl message.
This gives total freedom around key handling.

Users are free to program such function the way they think is best,
allowing them to support any private key storage
or to delegate signature to any external service or device.

Most notably: this allows to implement custom access to any HSM device.
Do not use undocumented OpenSSL "implicit param", rather be explicit about what
PSS params that where negotiated.
@IngelaAndin
Copy link
Contributor Author

@dgud rereview doc and added tests

@IngelaAndin IngelaAndin merged commit 4836605 into erlang:master Dec 12, 2023
17 checks passed
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 testing currently being tested, tag is used by OTP internal CI
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants