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

support mbedtls as backend #495

Merged
merged 85 commits into from
Nov 16, 2023
Merged

support mbedtls as backend #495

merged 85 commits into from
Nov 16, 2023

Conversation

kazuho
Copy link
Member

@kazuho kazuho commented Nov 14, 2023

Subsumes #485.

@huitema
Copy link
Collaborator

huitema commented Nov 14, 2023

@kazuho: thanks for picking up this PR. Is there a way I can see the differences from #485? That might help me improve the next PR...

It seems that we have an issue with the cmake discovery. Do you want me to try debug it?

@kazuho kazuho marked this pull request as ready for review November 15, 2023 06:14
Copy link
Member Author

@kazuho kazuho left a comment

Choose a reason for hiding this comment

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

@huitema Sorry for the delay.

Probably the best way to see the differences would be to run git diff -M10 -w -r cc9364e..7e7d39b.

But my focus has been to minimize the cost of maintaining another backend. I've moved comments to picotls.h, changed tests so that existing ones will be reused, etc.

Comments below explain some of the big changes that I have made.

cmake/FindMbedTLS.cmake Show resolved Hide resolved
cmake/FindMbedTLS.cmake Outdated Show resolved Hide resolved
.github/workflows/ci.yml Show resolved Hide resolved
lib/mbedtls.c Show resolved Hide resolved
lib/mbedtls.c Show resolved Hide resolved
ctx->super.do_set_iv = aead_set_iv;
if (is_enc) {
ctx->super.do_encrypt = ptls_aead__do_encrypt;
ctx->super.do_encrypt_v = aead_encrypt_v;
Copy link
Member Author

Choose a reason for hiding this comment

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

The implementation has been simplified by dropping support for streaming encryption (~init -> ~update -> ~final). We only support ptls_aead_encrypt and ptls_aead_encrypt_v.

These changes have led to reduction of states in ptls_mbedtls_aead_context_t.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Is that something we expect for every AEAD implementation?

Copy link
Member Author

Choose a reason for hiding this comment

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

API for streaming encryption has been deprecated, as stated in picotls.h (here and here). This PR expands the explanation a bit.

Backends may support streaming encryption. So if you want to have that supported in the mbedtls backend, please say so.

lib/mbedtls.c Show resolved Hide resolved
t/ptlsbench.c Show resolved Hide resolved
Copy link
Collaborator

@huitema huitema left a comment

Choose a reason for hiding this comment

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

Only one question left: should we write some documentation on how to use MbedTLS? or, should we wait until we have ported the signature and verify functions?

.github/workflows/ci.yml Show resolved Hide resolved
@kazuho kazuho merged commit dfe6072 into master Nov 16, 2023
13 checks passed
@kazuho
Copy link
Member Author

kazuho commented Nov 16, 2023

@huitema Thank you for reviews. This PR has been merged.

Only one question left: should we write some documentation on how to use MbedTLS? or, should we wait until we have ported the signature and verify functions?

I think we can add docs to https://github.com/h2o/picotls/wiki now that this PR has been merged. I can see the docs written in two ways. We can expand https://github.com/h2o/picotls/wiki/Using-picotls as it talks about two backends that we have already. Or, we can create a new document dedicated mbedtls backend. I'm fine either ways, but maybe the latter would be simpler and easier to understand?

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.

2 participants