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

Add a tutorial on writing a simple blocking TLS client #21133

Closed
wants to merge 9 commits into from

Conversation

mattcaswell
Copy link
Member

Provide guidance on the steps needed to write a very simple blocking TLS
client.

This blocking client is intended to be used to explain how to implement
a simple client in the documentation.
@mattcaswell mattcaswell added branch: master Merge to master branch approval: review pending This pull request needs review by a committer approval: otc review pending This pull request needs review by an OTC member tests: exempted The PR is exempt from requirements for testing labels Jun 6, 2023
@mattcaswell
Copy link
Member Author

@vdukhovni - I'd appreciate your input on this (in particular the demo)

Provide guidance on the steps needed to write a very simple blocking TLS
client.
@arapov arapov linked an issue Jun 6, 2023 that may be closed by this pull request
Copy link
Contributor

@paulidale paulidale left a comment

Choose a reason for hiding this comment

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

Really good stuff.

My main question is if the background material belongs here or elsewhere. My feeling is elsewhere.

demos/guide/Makefile Outdated Show resolved Hide resolved
doc/man7/guide-tls-client-block.pod Outdated Show resolved Hide resolved
doc/man7/guide-tls-client-block.pod Outdated Show resolved Hide resolved
doc/man7/guide-tls-client-block.pod Outdated Show resolved Hide resolved
Copy link

@vdukhovni vdukhovni left a comment

Choose a reason for hiding this comment

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

Perhaps also set the min_protocol to TLSv1.2 as BCP for non-opportunistic TLS, and to showcase that API.

demos/guide/tls-client-block.c Outdated Show resolved Hide resolved
*/
ctx = SSL_CTX_new(TLS_client_method());
if (ctx == NULL) {
printf("Failed to create the SSL_CTX\n");

Choose a reason for hiding this comment

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

I would use a wrapper function that prints the error stack.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm not sure it adds much? The error stack printing is already there and in the common "end" block. The only thing different about each location is printing the error message itself.

Choose a reason for hiding this comment

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

Yes, HTTP?1.0 is supposed to close by default, but AFAIK sending Connection: close is still recommended, but I'm not sure how useful this is in practice.

/* Create an SSL object to represent the TLS connection */
ssl = SSL_new(ctx);
if (ssl == NULL) {
printf("Failed to create the SSL object\n");

Choose a reason for hiding this comment

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

Call same wrapper to dump the error stack and then print the message. Here and below as appropriate.

Copy link
Member Author

Choose a reason for hiding this comment

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

As above.

* Virtually all clients should do this unless you really know what you
* are doing.
*/
if (!SSL_set1_host(ssl, HOSTNAME)) {

Choose a reason for hiding this comment

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

Perhaps mention that in some cases it may be appropriate to disable wildcard matching mentioning the relevant manpage.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'd prefer not to in this example. I'm trying to keep it simple and the wildcard handling is probably not something most users need to be worrying about.

}

/* Write an HTTP GET request to the peer */
if (!SSL_write_ex(ssl, request, strlen(request), &written)) {

Choose a reason for hiding this comment

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

This is the blocking API, with no WANT_READ/WANT_WRITE. It may be also good to have examples that use a non-blocking BIO poll for I/O completion/ready.

Copy link
Member Author

Choose a reason for hiding this comment

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

I plan to do provide a specific example and associated tutorial for non-blocking in a future PR.

* non-printable or have NUL characters in the middle of it.
*/
buf[readbytes] = '\0';
printf("%s", buf);

Choose a reason for hiding this comment

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

Could just use fwrite(3) to dump readbytes bytes of the response, and then putchar('\n').

Choose a reason for hiding this comment

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

Also, what if the server does not close the connection as soon as the response is sent? Perhaps also include a "Connection: close\r\n" request header?

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm doing HTTP/1.0 here. IIUC, in HTTP/1.0 the default expectation is that the connection is closed automatically. The "Connection: close" header is an HTTP/1.1 header so shouldn't be needed. Or have I misunderstood something?

For tutorial type pages it doesn't make any sense to have a DESCRIPTION
section.
We split the page into two: one covering basic TLS introductory material
that applies to both clients and servers, and one with the specific
material on writing a blocking TLS client.
@mattcaswell
Copy link
Member Author

Perhaps also set the min_protocol to TLSv1.2 as BCP for non-opportunistic TLS, and to showcase that API.

Done

@mattcaswell
Copy link
Member Author

All feedback addressed or otherwise responded to above. Please take another look.

@paulidale paulidale removed the approval: otc review pending This pull request needs review by an OTC member label Jun 7, 2023
@paulidale
Copy link
Contributor

@vdukhovni you should probably reappove after the changes.

/* Helper function to create a BIO connected to the server */
static BIO *create_socket_bio(const char *hostname, const char *port)
{
int sock = -1;

Choose a reason for hiding this comment

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

On Windows the socket type is a HANDLE, not an int. Is this code supposed to be Windows-portable?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, that is an interesting anomaly. However BIO_socket() and friends return an int - so this code is correct. InternallyBIO_socket() implicitly casts the HANDLE to int. But its been that way for a very long time and is used (for example in s_client and s_server). So in practice I don't think this is actually a problem. Either way it isn't a problem for this demo code because it is using the API as it is designed.

*/
ctx = SSL_CTX_new(TLS_client_method());
if (ctx == NULL) {
printf("Failed to create the SSL_CTX\n");

Choose a reason for hiding this comment

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

Yes, HTTP?1.0 is supposed to close by default, but AFAIK sending Connection: close is still recommended, but I'm not sure how useful this is in practice.

@mattcaswell
Copy link
Member Author

Yes, HTTP?1.0 is supposed to close by default, but AFAIK sending Connection: close is still recommended, but I'm not sure how useful this is in practice.

Ok. I added the "Connection: close" header. It may be redundant, but at least it looks harmless.

@mattcaswell
Copy link
Member Author

I've pushed a fixup to add the "Connection: close" header. I also spotted a minor bug in my socket creation code (missing "break" in the for loop) which I also fixed.

So - please could I have one more round of reconfirmations? @vdukhovni, @paulidale.

@hlandau
Copy link
Member

hlandau commented Jun 12, 2023

@mattcaswell Needs rebase over check-ansi fix

@t8m t8m closed this Jun 12, 2023
@t8m t8m reopened this Jun 12, 2023
@t8m
Copy link
Member

t8m commented Jun 12, 2023

@mattcaswell Needs rebase over check-ansi fix

It is sufficient to close&reopen so the test is re-run.

@paulidale paulidale added approval: done This pull request has the required number of approvals and removed approval: review pending This pull request needs review by a committer labels Jun 13, 2023
@openssl-machine openssl-machine added approval: ready to merge The 24 hour grace period has passed, ready to merge and removed approval: done This pull request has the required number of approvals labels Jun 14, 2023
@openssl-machine
Copy link
Collaborator

This pull request is ready to merge

@paulidale
Copy link
Contributor

Merged to master.

@paulidale paulidale closed this Jun 14, 2023
openssl-machine pushed a commit that referenced this pull request Jun 14, 2023
This blocking client is intended to be used to explain how to implement
a simple client in the documentation.

Reviewed-by: Viktor Dukhovni <viktor@openssl.org>
Reviewed-by: Paul Dale <pauli@openssl.org>
(Merged from #21133)
openssl-machine pushed a commit that referenced this pull request Jun 14, 2023
Provide guidance on the steps needed to write a very simple blocking TLS
client.

Reviewed-by: Viktor Dukhovni <viktor@openssl.org>
Reviewed-by: Paul Dale <pauli@openssl.org>
(Merged from #21133)
openssl-machine pushed a commit that referenced this pull request Jun 14, 2023
For tutorial type pages it doesn't make any sense to have a DESCRIPTION
section.

Reviewed-by: Viktor Dukhovni <viktor@openssl.org>
Reviewed-by: Paul Dale <pauli@openssl.org>
(Merged from #21133)
openssl-machine pushed a commit that referenced this pull request Jun 14, 2023
We split the page into two: one covering basic TLS introductory material
that applies to both clients and servers, and one with the specific
material on writing a blocking TLS client.

Reviewed-by: Viktor Dukhovni <viktor@openssl.org>
Reviewed-by: Paul Dale <pauli@openssl.org>
(Merged from #21133)
openssl-machine pushed a commit that referenced this pull request Jun 14, 2023
Reviewed-by: Viktor Dukhovni <viktor@openssl.org>
Reviewed-by: Paul Dale <pauli@openssl.org>
(Merged from #21133)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approval: ready to merge The 24 hour grace period has passed, ready to merge branch: master Merge to master branch tests: exempted The PR is exempt from requirements for testing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Writing a simple blocking TLS Client
6 participants