Skip to content

Asynchronous signing#408

Closed
sharksforarms wants to merge 24 commits intoh2o:masterfrom
sharksforarms:sharksforarms/async
Closed

Asynchronous signing#408
sharksforarms wants to merge 24 commits intoh2o:masterfrom
sharksforarms:sharksforarms/async

Conversation

@sharksforarms
Copy link
Copy Markdown
Contributor

@sharksforarms sharksforarms commented Jul 21, 2022

For: h2o/h2o#3064

Splits the certificate and certificate verify function and adds a PTLS_STATE_SERVER_GENERATING_CERTIFICATE_VERIFY state and PTLS_ERROR_ASYNC_OPERATION return code

References:
#291

signing can return `PTLS_ERROR_ASYNC_OPERATION` which will
set the TLS state to `PTLS_STATE_SERVER_GENERATING_CERTIFICATE_VERIFY`

It is up to the contract between the crypto implementation/application to
notify and retry `ptls_handshake`
in the case of asynchronous operation we are waiting on data,
a new buffer cannot be re-used
"The data pointed to by args and of size size will be copied and then passed as an argument to func when the job starts"
https://www.openssl.org/docs/man1.1.1/man3/ASYNC_start_job.html
@sharksforarms sharksforarms marked this pull request as draft July 21, 2022 20:34
Copy link
Copy Markdown
Member

@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.

Thank you for the changes. Looks good, I only have a few questions.

Comment thread include/picotls.h Outdated
Comment thread lib/picotls.c
}
if (tls->server.sign_certificate.cb != NULL) {
tls->server.sign_certificate.cb(tls->server.sign_certificate.sign_certificate_ctx);
}
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Am I correct to understand that the intent of the design is to invoke this callback only when the operation is to be cancelled?

Assuming that is the case,

  • Could you point me to the code that clears the callback when the signature is obtained from the backend? I think that's necessary because once the operation is complete async context would be destroyed.
  • It might make sense to call this callback cancel_cb or similar, as the only role of the callback is cancellation.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

That's correct.

The callback is being set and cleared in the backend implementation:

The cancel callback is set when an async job is started (ASYNC_PAUSE) https://github.com/sharksforarms/picotls/blob/sharksforarms/async/lib/openssl.c#L861 and cleared at the end of the function (on error or job finished) https://github.com/sharksforarms/picotls/blob/sharksforarms/async/lib/openssl.c#L901

Comment thread lib/picotls.c Outdated
Comment thread include/picotls.h Outdated
/**
* boolean indicating if handshaking should be asynchronous
*/
unsigned async_handshake : 1;
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

How about containing this state within the openssl backend? It can be a flag of ptls_openssl_sign_certificate_t. If a backend can calculate synchrously (or asynchronously) depends on each backend. To give an example, a crypto token device can only do asynchronous operation.

IMO, all we need is a mechanism that allows the backend signal picotls if the operation has started asynchronously, the capability being provided by the error code being added.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yes you're correct here, I don't think this flag should be needed in the end. I've added it as a way to temporarily disable async, for example in QUIC context, quicly uses a different code path, which I haven't gotten around to yet.

@kazuho kazuho mentioned this pull request Sep 26, 2022
@kazuho
Copy link
Copy Markdown
Member

kazuho commented Oct 6, 2022

Subsumed by #422.

@kazuho kazuho closed this Oct 6, 2022
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