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 TLS-ALPN preference setting support #130

Closed
wants to merge 22 commits into
base: master
from

Conversation

Projects
None yet
3 participants
@jamesnvc
Contributor

jamesnvc commented Aug 9, 2018

To address issue #129

@jamesnvc

This comment has been minimized.

Show comment
Hide comment
@jamesnvc

jamesnvc Aug 10, 2018

Contributor

Tested functionality by running a test SSL server with the following commands:

# Generate a test SSL certificate
openssl req -x509 -newkey rsa:4096 -keyout key.pem -out server.pem -days 365 -nodes
# start a test ssl server
openssl s_server -key key.pem -alpn h2

then running the following code:

test :-
    ssl_context(client, Ctx0, [host(localhost),
                               close_parent(true),
                               cert_verify_hook(cert_accept_any)]),
    ssl_set_alpn_protos(Ctx0, Ctx, [h2]),
    tcp_connect('127.0.0.1':4433, PlainStreamPair, []),
    stream_pair(PlainStreamPair, PlainRead, PlainWrite),
    ssl_negotiate(Ctx, PlainRead, PlainWrite,
                  SSLRead, SSLWrite).

?- test.

And observing the line ALPN protocols advertised by the client: h2 in the output of the OpenSSL server.

Contributor

jamesnvc commented Aug 10, 2018

Tested functionality by running a test SSL server with the following commands:

# Generate a test SSL certificate
openssl req -x509 -newkey rsa:4096 -keyout key.pem -out server.pem -days 365 -nodes
# start a test ssl server
openssl s_server -key key.pem -alpn h2

then running the following code:

test :-
    ssl_context(client, Ctx0, [host(localhost),
                               close_parent(true),
                               cert_verify_hook(cert_accept_any)]),
    ssl_set_alpn_protos(Ctx0, Ctx, [h2]),
    tcp_connect('127.0.0.1':4433, PlainStreamPair, []),
    stream_pair(PlainStreamPair, PlainRead, PlainWrite),
    ssl_negotiate(Ctx, PlainRead, PlainWrite,
                  SSLRead, SSLWrite).

?- test.

And observing the line ALPN protocols advertised by the client: h2 in the output of the OpenSSL server.

@triska

This comment has been minimized.

Show comment
Hide comment
@triska

triska Aug 10, 2018

Member

Please augment ssl_set_options/3 instead!

Member

triska commented Aug 10, 2018

Please augment ssl_set_options/3 instead!

@jamesnvc

This comment has been minimized.

Show comment
Hide comment
@jamesnvc

jamesnvc Aug 10, 2018

Contributor
Contributor

jamesnvc commented Aug 10, 2018

@triska

This comment has been minimized.

Show comment
Hide comment
@triska

triska Aug 10, 2018

Member

For library(ssl), following the C API directly is typically not a good idea.

Please let me know any time if you have any questions about API design for library(ssl)! I am looking forward to working together with you on this.

Member

triska commented Aug 10, 2018

For library(ssl), following the C API directly is typically not a good idea.

Please let me know any time if you have any questions about API design for library(ssl)! I am looking forward to working together with you on this.

@triska

This comment has been minimized.

Show comment
Hide comment
@triska

triska Aug 10, 2018

Member

Very nice! If possible, please also make it compile if the API is not available (add configure checks).

You can rebase and force push to the branch to retain only the eventual patch.

Member

triska commented Aug 10, 2018

Very nice! If possible, please also make it compile if the API is not available (add configure checks).

You can rebase and force push to the branch to retain only the eventual patch.

@jamesnvc

This comment has been minimized.

Show comment
Hide comment
@jamesnvc

jamesnvc Aug 10, 2018

Contributor

Okay, adding the configure checks; thank-you very much for your quick feedback!

Contributor

jamesnvc commented Aug 10, 2018

Okay, adding the configure checks; thank-you very much for your quick feedback!

@triska

This comment has been minimized.

Show comment
Hide comment
@triska

triska Aug 10, 2018

Member

Thank you for working on this! I think only one API call needs to be guarded. You can simply make it a no-op if the API is not available, reducing the ifdefs.

Since this is a user-visible option, please also think a lot about the name. For example, do you like alpn_protocols?

Member

triska commented Aug 10, 2018

Thank you for working on this! I think only one API call needs to be guarded. You can simply make it a no-op if the API is not available, reducing the ifdefs.

Since this is a user-visible option, please also think a lot about the name. For example, do you like alpn_protocols?

@jamesnvc

This comment has been minimized.

Show comment
Hide comment
@jamesnvc

jamesnvc Aug 10, 2018

Contributor

Alright, I will just make the actual function call ifdef-guarded.

Sure, I think alpn_protocols is a better name; I will make that change as well.

Contributor

jamesnvc commented Aug 10, 2018

Alright, I will just make the actual function call ifdef-guarded.

Sure, I think alpn_protocols is a better name; I will make that change as well.

Add TLS-ALPN preference setting support
Add pldoc

Free list of protocols after use

Set ALPN config in set_options instead of a separate function

Guard against SSL_CTX_set_alpn_protos being available

Remove extraneous ifdefs & change atom name to `alpn_protocols`
@jamesnvc

This comment has been minimized.

Show comment
Hide comment
@jamesnvc

jamesnvc Aug 10, 2018

Contributor

Squashed & pushed; I am about to leave town for the weekend, so I won't be able to respond or make more changes until Tuesday. Thanks again for all the feedback!

Contributor

jamesnvc commented Aug 10, 2018

Squashed & pushed; I am about to leave town for the weekend, so I won't be able to respond or make more changes until Tuesday. Thanks again for all the feedback!

@triska

This comment has been minimized.

Show comment
Hide comment
@triska

triska Aug 10, 2018

Member

Very nice, it's almost perfect! Please do one final pass thinking about edge-cases, like malloc failing, and add the appropriate safety checks. After that, it's ready for merging!

However, what about the hook for ALPN? How should it best be integrated. Personally, I have found it useful to wait for a few days before committing changes that integrate the OpenSSL API, since the C-level API is so bad, and we can often come up with something that is more useful. So, before merging, I would recommend to take a few days to think this fully through, and I hope you are OK with this. Thank you for this nice patch!

Member

triska commented Aug 10, 2018

Very nice, it's almost perfect! Please do one final pass thinking about edge-cases, like malloc failing, and add the appropriate safety checks. After that, it's ready for merging!

However, what about the hook for ALPN? How should it best be integrated. Personally, I have found it useful to wait for a few days before committing changes that integrate the OpenSSL API, since the C-level API is so bad, and we can often come up with something that is more useful. So, before merging, I would recommend to take a few days to think this fully through, and I hope you are OK with this. Thank you for this nice patch!

@jamesnvc

This comment has been minimized.

Show comment
Hide comment
@jamesnvc

jamesnvc Aug 14, 2018

Contributor

Thinking about how the server can hook in still, but I did implement what I think would be useful in most cases; if the alpn_protocols option is set for a server, then it will set a callback that uses the provided OpenSSL helper function to find the common protocol that client & server share. I suspect that this is the most common use-case for ALPN & hope that it might obviate the need to make a hook predicate for most cases.

I suppose that some way to determine the negotiated protocol would be needed as well, which I'm currently thinking would just be a separate predicate wrapping SSL_get0_alpn_selected().

To test server implementation:

# generate keys
openssl req -x509 -newkey rsa:4096 -keyout key.pem -out server.pem -days 365 -nodes

start test server

test_server :-
    tcp_socket(Socket),
    tcp_bind(Socket, 4433),
    tcp_listen(Socket, 5),
    tcp_open_socket(Socket, AcceptFd, _),
    tcp_accept(AcceptFd, Socket2, _Peer),
    tcp_open_socket(Socket2, PlainStreamPair),
    stream_pair(PlainStreamPair, PlainRead, PlainWrite),
    ssl_context(server, Ctx, [alpn_protocols([spdy, h2]),
                              certificate_file('PATH TO CERT.PEM'),
                              key_file('PATH TO KEY.PEM')]),
    ssl_negotiate(Ctx, PlainRead, PlainWrite, SSLRead, SSLWrite),
    stream_pair(SSLStream, SSLRead, SSLWrite),
    put_codes(SSLStream, `hello there`),
    close(SSLStream),
    tcp_close_socket(Socket).

Use OpenSSL client, verify that ALPN protocol: h2 line appears

openssl s_client -CAfile server.pem -alpn h2,h2c
Contributor

jamesnvc commented Aug 14, 2018

Thinking about how the server can hook in still, but I did implement what I think would be useful in most cases; if the alpn_protocols option is set for a server, then it will set a callback that uses the provided OpenSSL helper function to find the common protocol that client & server share. I suspect that this is the most common use-case for ALPN & hope that it might obviate the need to make a hook predicate for most cases.

I suppose that some way to determine the negotiated protocol would be needed as well, which I'm currently thinking would just be a separate predicate wrapping SSL_get0_alpn_selected().

To test server implementation:

# generate keys
openssl req -x509 -newkey rsa:4096 -keyout key.pem -out server.pem -days 365 -nodes

start test server

test_server :-
    tcp_socket(Socket),
    tcp_bind(Socket, 4433),
    tcp_listen(Socket, 5),
    tcp_open_socket(Socket, AcceptFd, _),
    tcp_accept(AcceptFd, Socket2, _Peer),
    tcp_open_socket(Socket2, PlainStreamPair),
    stream_pair(PlainStreamPair, PlainRead, PlainWrite),
    ssl_context(server, Ctx, [alpn_protocols([spdy, h2]),
                              certificate_file('PATH TO CERT.PEM'),
                              key_file('PATH TO KEY.PEM')]),
    ssl_negotiate(Ctx, PlainRead, PlainWrite, SSLRead, SSLWrite),
    stream_pair(SSLStream, SSLRead, SSLWrite),
    put_codes(SSLStream, `hello there`),
    close(SSLStream),
    tcp_close_socket(Socket).

Use OpenSSL client, verify that ALPN protocol: h2 line appears

openssl s_client -CAfile server.pem -alpn h2,h2c
@jamesnvc

This comment has been minimized.

Show comment
Hide comment
@jamesnvc

jamesnvc Aug 14, 2018

Contributor

Oh, or I suppose instead of a separate predicate, I could make ssl_session/2 include the negotiated ALPN if applicable.

Contributor

jamesnvc commented Aug 14, 2018

Oh, or I suppose instead of a separate predicate, I could make ssl_session/2 include the negotiated ALPN if applicable.

@triska

This comment has been minimized.

Show comment
Hide comment
@triska

triska Aug 14, 2018

Member

Yes, that would be a good solution!

Member

triska commented Aug 14, 2018

Yes, that would be a good solution!

@jamesnvc

This comment has been minimized.

Show comment
Hide comment
@jamesnvc

jamesnvc Aug 14, 2018

Contributor

Okay, ssl_session/2 now also has a alpn_protocol functor, which is the string of the negotiated protocol.

Contributor

jamesnvc commented Aug 14, 2018

Okay, ssl_session/2 now also has a alpn_protocol functor, which is the string of the negotiated protocol.

@triska

This comment has been minimized.

Show comment
Hide comment
@triska

triska Aug 14, 2018

Member

Wow, awesome work, thank you a lot! I suggest that I merge this very soon, i.e., within the next few days, if nothing else occurs.

Making the callback configurable may be worth thinking about. The SNI hook may provide some inspiration, please see the sni_hook/1 option if you are interested in working on this.

Member

triska commented Aug 14, 2018

Wow, awesome work, thank you a lot! I suggest that I merge this very soon, i.e., within the next few days, if nothing else occurs.

Making the callback configurable may be worth thinking about. The SNI hook may provide some inspiration, please see the sni_hook/1 option if you are interested in working on this.

@jamesnvc

This comment has been minimized.

Show comment
Hide comment
@jamesnvc

jamesnvc Aug 14, 2018

Contributor

Excellent, thank you! I will have a look at making the callback configurable too.

Contributor

jamesnvc commented Aug 14, 2018

Excellent, thank you! I will have a look at making the callback configurable too.

@jamesnvc

This comment has been minimized.

Show comment
Hide comment
@jamesnvc

jamesnvc Aug 17, 2018

Contributor

Okay! There is now the option for the server to provide a hook to determine the protocol. Example usage, assuming cert files generated as above:

Server:

protos_hook(Protos, Selected) :-
    debug(xxx, "PROTOS ~w", [Protos]),
    random_member(Selected, Protos).

test_server :-
    tcp_socket(Socket),
    tcp_bind(Socket, 4433),
    tcp_listen(Socket, 5),
    tcp_open_socket(Socket, AcceptFd, _),
    tcp_accept(AcceptFd, Socket2, _Peer),
    tcp_open_socket(Socket2, PlainStreamPair),
    stream_pair(PlainStreamPair, PlainRead, PlainWrite),
    ssl_context(server, Ctx, [alpn_protocol_hook(protos_hook),
                              close_parent(true),
                              certificate_file('PATH TO CERT.PEM'),
                              key_file('PATH TO KEY.PEM')]),
    ssl_negotiate(Ctx, PlainRead, PlainWrite, SSLRead, SSLWrite),
    stream_pair(SSLStream, SSLRead, SSLWrite),
    ssl_session(SSLStream, Session),
    debug(xxx, "Session info ~k", [Session]),
    close(SSLStream),
    tcp_close_socket(Socket).

Client:

openssl s_client -CAfile server.pem -alpn h2,h2c

When run multiple times, it can be seen that the negotiated ALPN protocol is being randomly selected by the provided hook.

Contributor

jamesnvc commented Aug 17, 2018

Okay! There is now the option for the server to provide a hook to determine the protocol. Example usage, assuming cert files generated as above:

Server:

protos_hook(Protos, Selected) :-
    debug(xxx, "PROTOS ~w", [Protos]),
    random_member(Selected, Protos).

test_server :-
    tcp_socket(Socket),
    tcp_bind(Socket, 4433),
    tcp_listen(Socket, 5),
    tcp_open_socket(Socket, AcceptFd, _),
    tcp_accept(AcceptFd, Socket2, _Peer),
    tcp_open_socket(Socket2, PlainStreamPair),
    stream_pair(PlainStreamPair, PlainRead, PlainWrite),
    ssl_context(server, Ctx, [alpn_protocol_hook(protos_hook),
                              close_parent(true),
                              certificate_file('PATH TO CERT.PEM'),
                              key_file('PATH TO KEY.PEM')]),
    ssl_negotiate(Ctx, PlainRead, PlainWrite, SSLRead, SSLWrite),
    stream_pair(SSLStream, SSLRead, SSLWrite),
    ssl_session(SSLStream, Session),
    debug(xxx, "Session info ~k", [Session]),
    close(SSLStream),
    tcp_close_socket(Socket).

Client:

openssl s_client -CAfile server.pem -alpn h2,h2c

When run multiple times, it can be seen that the negotiated ALPN protocol is being randomly selected by the provided hook.

@triska

This comment has been minimized.

Show comment
Hide comment
@triska

triska Aug 17, 2018

Member

This is awesome!

Let's make one more pass thinking about the semantics of the hook, in particular the arguments it should receive. For example, do you think it would be useful to also pass it the SSL context? Or any further arguments?

The convention throughout most of library(ssl) is to pass the context in the first argument, if it occurs.

Member

triska commented Aug 17, 2018

This is awesome!

Let's make one more pass thinking about the semantics of the hook, in particular the arguments it should receive. For example, do you think it would be useful to also pass it the SSL context? Or any further arguments?

The convention throughout most of library(ssl) is to pass the context in the first argument, if it occurs.

@jamesnvc

This comment has been minimized.

Show comment
Hide comment
@jamesnvc

jamesnvc Aug 17, 2018

Contributor

Sure, I was thinking that it might make sense to pass the context in, just so the hook can determine which context the hook is being called in. Other than that, I think just the client protocols in & the selected protocol out should be sufficient -- in the place where the hook is being called, those seem like the only relevant data & the only reasonable response (the predicate failing will result in no ALPN being negotiated).

Contributor

jamesnvc commented Aug 17, 2018

Sure, I was thinking that it might make sense to pass the context in, just so the hook can determine which context the hook is being called in. Other than that, I think just the client protocols in & the selected protocol out should be sufficient -- in the place where the hook is being called, those seem like the only relevant data & the only reasonable response (the predicate failing will result in no ALPN being negotiated).

@jamesnvc

This comment has been minimized.

Show comment
Hide comment
@jamesnvc

jamesnvc Aug 17, 2018

Contributor

Above example now works with the hook taking the context in the first argument:

protos_hook(Ctx, Protos, Selected) :-
    debug(xxx, "CTX ~w PROTOS ~w", [Ctx, Protos]),
    random_member(Selected, Protos).
Contributor

jamesnvc commented Aug 17, 2018

Above example now works with the hook taking the context in the first argument:

protos_hook(Ctx, Protos, Selected) :-
    debug(xxx, "CTX ~w PROTOS ~w", [Ctx, Protos]),
    random_member(Selected, Protos).
@JanWielemaker

This comment has been minimized.

Show comment
Hide comment
@JanWielemaker

JanWielemaker Aug 20, 2018

Member

Hi James, Markus. Thank you both for looking into this! I've been on holidays. Can one of you summarise the status and/or ping me if action is required from my side?

Member

JanWielemaker commented Aug 20, 2018

Hi James, Markus. Thank you both for looking into this! I've been on holidays. Can one of you summarise the status and/or ping me if action is required from my side?

@jamesnvc

This comment has been minimized.

Show comment
Hide comment
@jamesnvc

jamesnvc Aug 20, 2018

Contributor

Hi Jan! I think everything is implemented now: This now allows a client to specify a list of desired ALPN protocols, the server to either specify a list (in which case the first-common protocol will be negotiated) or a hook predicate to negotiate a protocol, and for both client & server to see the negotiated protocol (if any) via ssl_session/2.

Contributor

jamesnvc commented Aug 20, 2018

Hi Jan! I think everything is implemented now: This now allows a client to specify a list of desired ALPN protocols, the server to either specify a list (in which case the first-common protocol will be negotiated) or a hook predicate to negotiate a protocol, and for both client & server to see the negotiated protocol (if any) via ssl_session/2.

@triska

This comment has been minimized.

Show comment
Hide comment
@triska

triska Aug 20, 2018

Member

I agree with James that this is ready for merging. Awesome job, thank you James!

Member

triska commented Aug 20, 2018

I agree with James that this is ready for merging. Awesome job, thank you James!

@JanWielemaker

This comment has been minimized.

Show comment
Hide comment
@JanWielemaker

JanWielemaker Aug 21, 2018

Member

Great! I'm away for a couple of days and will look into this end of this week or early next week.

Member

JanWielemaker commented Aug 21, 2018

Great! I'm away for a couple of days and will look into this end of this week or early next week.

@triska

This comment has been minimized.

Show comment
Hide comment
@triska

triska Aug 21, 2018

Member

I think that is good timing, because one issue actually still remains: ALPN provides us with the opportunity to use different certificates based on the negotiated protocol. Yet, in its current form, the hook cannot be used to provide this flexibility.

James, what do you think: Would it make sense to extend the hook predicate with one additional argument, where application programmers can specify an SSL context that is used to negotiate the eventual connection, in analogy to sni_hook?

Member

triska commented Aug 21, 2018

I think that is good timing, because one issue actually still remains: ALPN provides us with the opportunity to use different certificates based on the negotiated protocol. Yet, in its current form, the hook cannot be used to provide this flexibility.

James, what do you think: Would it make sense to extend the hook predicate with one additional argument, where application programmers can specify an SSL context that is used to negotiate the eventual connection, in analogy to sni_hook?

@jamesnvc

This comment has been minimized.

Show comment
Hide comment
@jamesnvc

jamesnvc Aug 21, 2018

Contributor

Alright! The hook now allows setting a context. Example, assuming two sets of certificates have been generated as above (presumably give the two certs different information so you can tell which one the client accepts):

protos_hook(Ctx0, Protos, Ctx1, Selected) :-
    read_file_to_codes('PATH TO SERVER2.PEM', ServerCert_, []),
    string_codes(ServerCert, ServerCert_),
    read_file_to_codes('PATH TO KEY2.PEM', KeyCert_, []),
    string_codes(KeyCert, KeyCert_),
    ssl_add_certificate_key(Ctx0,
                            ServerCert,
                            KeyCert,
                           Ctx1),
    debug(xxx, "CTX ~w -> ~w PROTOS ~w", [Ctx0, Ctx1, Protos]),
    random_member(Selected, Protos).

test_server :-
    tcp_socket(Socket),
    tcp_bind(Socket, 4433),
    tcp_listen(Socket, 5),
    tcp_open_socket(Socket, AcceptFd, _),
    tcp_accept(AcceptFd, Socket2, _Peer),
    tcp_open_socket(Socket2, PlainStreamPair),
    stream_pair(PlainStreamPair, PlainRead, PlainWrite),
    ssl_context(server, Ctx, [alpn_protocol_hook(protos_hook),
                              close_parent(true),
                              certificate_file('PATH TO SERVER1.PEM'),
                              key_file('PATH TO KEY1.PEM')]),
    ssl_negotiate(Ctx, PlainRead, PlainWrite, SSLRead, SSLWrite),
    stream_pair(SSLStream, SSLRead, SSLWrite),
    ssl_session(SSLStream, Session),
    debug(xxx, "Session info ~k", [Session]),
    close(SSLStream),
    tcp_close_socket(Socket).

Then using openssl s_client as above, one can see the certificate is being set from the hook.

Contributor

jamesnvc commented Aug 21, 2018

Alright! The hook now allows setting a context. Example, assuming two sets of certificates have been generated as above (presumably give the two certs different information so you can tell which one the client accepts):

protos_hook(Ctx0, Protos, Ctx1, Selected) :-
    read_file_to_codes('PATH TO SERVER2.PEM', ServerCert_, []),
    string_codes(ServerCert, ServerCert_),
    read_file_to_codes('PATH TO KEY2.PEM', KeyCert_, []),
    string_codes(KeyCert, KeyCert_),
    ssl_add_certificate_key(Ctx0,
                            ServerCert,
                            KeyCert,
                           Ctx1),
    debug(xxx, "CTX ~w -> ~w PROTOS ~w", [Ctx0, Ctx1, Protos]),
    random_member(Selected, Protos).

test_server :-
    tcp_socket(Socket),
    tcp_bind(Socket, 4433),
    tcp_listen(Socket, 5),
    tcp_open_socket(Socket, AcceptFd, _),
    tcp_accept(AcceptFd, Socket2, _Peer),
    tcp_open_socket(Socket2, PlainStreamPair),
    stream_pair(PlainStreamPair, PlainRead, PlainWrite),
    ssl_context(server, Ctx, [alpn_protocol_hook(protos_hook),
                              close_parent(true),
                              certificate_file('PATH TO SERVER1.PEM'),
                              key_file('PATH TO KEY1.PEM')]),
    ssl_negotiate(Ctx, PlainRead, PlainWrite, SSLRead, SSLWrite),
    stream_pair(SSLStream, SSLRead, SSLWrite),
    ssl_session(SSLStream, Session),
    debug(xxx, "Session info ~k", [Session]),
    close(SSLStream),
    tcp_close_socket(Socket).

Then using openssl s_client as above, one can see the certificate is being set from the hook.

@JanWielemaker

This comment has been minimized.

Show comment
Hide comment
@JanWielemaker

JanWielemaker Aug 23, 2018

Member

Hi James,

Did a first check and some editing. You did a good job. You find my version on the alpn branch at https://github.com/SWI-Prolog/packages-ssl/tree/alpn

I did some layout consistency, added more checks on PL_* failure return (notably PL_open_foreign_frame() and PL_recorded() may fail if the stack is full). Changes some PL_get_atom_chars() to PL_get_nchars() to be more flexible, handle errors and consistently use UTF-8 (is this right; you now used it at one place?) There are two FIXMEs:

  • If we get an exception in the callback, we simply ignore it. That is bad. At least we could use PL_Q_NORMAL to tell Prolog we won't be handling the exception in C. Better is to pass it back to the Prolog predicate that caused the call-back to trigger. Can that be done? If we can't do that we should probably print the exception using pring_message/2.
  • The string returned from the callback used to be the string from an atom. I don't think anything prevents the atom to be GC'ed right away though. How long is this string assumed to live? Who is assumed to free it?

For my whole picture, this is just the SSL part. I understand you will continue with the HTTP side?

If we all agree on this I propose the squash the branch to a single commit with you attributed as the author. I think the intermediate commits are only confusing.

Member

JanWielemaker commented Aug 23, 2018

Hi James,

Did a first check and some editing. You did a good job. You find my version on the alpn branch at https://github.com/SWI-Prolog/packages-ssl/tree/alpn

I did some layout consistency, added more checks on PL_* failure return (notably PL_open_foreign_frame() and PL_recorded() may fail if the stack is full). Changes some PL_get_atom_chars() to PL_get_nchars() to be more flexible, handle errors and consistently use UTF-8 (is this right; you now used it at one place?) There are two FIXMEs:

  • If we get an exception in the callback, we simply ignore it. That is bad. At least we could use PL_Q_NORMAL to tell Prolog we won't be handling the exception in C. Better is to pass it back to the Prolog predicate that caused the call-back to trigger. Can that be done? If we can't do that we should probably print the exception using pring_message/2.
  • The string returned from the callback used to be the string from an atom. I don't think anything prevents the atom to be GC'ed right away though. How long is this string assumed to live? Who is assumed to free it?

For my whole picture, this is just the SSL part. I understand you will continue with the HTTP side?

If we all agree on this I propose the squash the branch to a single commit with you attributed as the author. I think the intermediate commits are only confusing.

@jamesnvc

This comment has been minimized.

Show comment
Hide comment
@jamesnvc

jamesnvc Aug 23, 2018

Contributor

Thank-you Jan!

...and consistently use UTF-8 (is this right; you now used it at one place?)

Oh yeah, I think it should be UTF-8. The SSL docs just say it's a byte vector, but the TLS-ALPN RFC says "This could be the UTF-8 encoding of the protocol name" (emphasis mine), so I guess that makes sense.

For the exception, I'm not sure if we can pass it to the calling predicate, since it's invoked by the SSL negotiate process, at which point I don't think we can get any reference to the calling Prolog context (I think?), so I guess print_message/2 is the way to go. I will implement that.

For the string being returned, on a closer reading of the SSL documentation, I think it's actually supposed to be one of the strings in the vector that was passed in, so the above code was only working because it would pick an atom from the in list. I will fix this shortly.
EDIT: It apparently works even if an unrelated atom is returned, so I'm not quite sure; I will in any case make it select from the in vector.

Regarding the HTTP part: Yup, I'm working in another repo on the HTTP/2 stuff; this was just the one bit that needs to happen at the TLS level. It's almost working...I will make it public soon, when I'm not quite as embarrassed by the state of it.

Indeed, once it's all ready, squashing sounds good.

Contributor

jamesnvc commented Aug 23, 2018

Thank-you Jan!

...and consistently use UTF-8 (is this right; you now used it at one place?)

Oh yeah, I think it should be UTF-8. The SSL docs just say it's a byte vector, but the TLS-ALPN RFC says "This could be the UTF-8 encoding of the protocol name" (emphasis mine), so I guess that makes sense.

For the exception, I'm not sure if we can pass it to the calling predicate, since it's invoked by the SSL negotiate process, at which point I don't think we can get any reference to the calling Prolog context (I think?), so I guess print_message/2 is the way to go. I will implement that.

For the string being returned, on a closer reading of the SSL documentation, I think it's actually supposed to be one of the strings in the vector that was passed in, so the above code was only working because it would pick an atom from the in list. I will fix this shortly.
EDIT: It apparently works even if an unrelated atom is returned, so I'm not quite sure; I will in any case make it select from the in vector.

Regarding the HTTP part: Yup, I'm working in another repo on the HTTP/2 stuff; this was just the one bit that needs to happen at the TLS level. It's almost working...I will make it public soon, when I'm not quite as embarrassed by the state of it.

Indeed, once it's all ready, squashing sounds good.

@JanWielemaker

This comment has been minimized.

Show comment
Hide comment
@JanWielemaker

JanWielemaker Aug 23, 2018

Member

For the exception, I'm not sure if we can pass it to the calling predicate, since it's invoked by the SSL negotiate process, at which point I don't think we can get any reference to the calling Prolog context (I think?), so I guess print_message/2 is the way to go. I will implement that.

So, we have ssl_negotiate/5 calling SSL functions that call our hook? If you just use PL_Q_PASS_EXCEPTION this causes the ssl function underlying ssl_negotiate to return failure (I guess) and this causes ssl_negotiate/5 to return FALSE to Prolog. If the exception is still there, Prolog will raise it. In other words, all should be fine as long as the only thing happening after the exception wrt. Prolog interaction is returning control back to Prolog using FALSE. There should also be no need to ask for the extended status code as you do not care whether the goal failed due to failure or an exception. Although, we may raise an exception inside the hook if the hook fails. I assume failure is not ok.

For the string being returned, on a closer reading of the SSL documentation, I think it's actually supposed to be one of the strings in the vector that was passed in, so the above code was only working because it would pick an atom from the in list. I will fix this shortly.

I guess there are two reliable options. One is to find the returned string in the original and return its index. The other is probably to add BUF_MALLOC to the PL_get_nchars() option, store the result in the context and free it when the context is destroyed.

Thanks for adding my commits to this pull request. That is probably the easiest way. Looking forward to HTTP/2!

Member

JanWielemaker commented Aug 23, 2018

For the exception, I'm not sure if we can pass it to the calling predicate, since it's invoked by the SSL negotiate process, at which point I don't think we can get any reference to the calling Prolog context (I think?), so I guess print_message/2 is the way to go. I will implement that.

So, we have ssl_negotiate/5 calling SSL functions that call our hook? If you just use PL_Q_PASS_EXCEPTION this causes the ssl function underlying ssl_negotiate to return failure (I guess) and this causes ssl_negotiate/5 to return FALSE to Prolog. If the exception is still there, Prolog will raise it. In other words, all should be fine as long as the only thing happening after the exception wrt. Prolog interaction is returning control back to Prolog using FALSE. There should also be no need to ask for the extended status code as you do not care whether the goal failed due to failure or an exception. Although, we may raise an exception inside the hook if the hook fails. I assume failure is not ok.

For the string being returned, on a closer reading of the SSL documentation, I think it's actually supposed to be one of the strings in the vector that was passed in, so the above code was only working because it would pick an atom from the in list. I will fix this shortly.

I guess there are two reliable options. One is to find the returned string in the original and return its index. The other is probably to add BUF_MALLOC to the PL_get_nchars() option, store the result in the context and free it when the context is destroyed.

Thanks for adding my commits to this pull request. That is probably the easiest way. Looking forward to HTTP/2!

@JanWielemaker

This comment has been minimized.

Show comment
Hide comment
@JanWielemaker

JanWielemaker Aug 23, 2018

Member

av+4 is not an atom but a term holding an atom. It isn't registered and doesn't need to be unregistered.

Member

JanWielemaker commented on ssl4pl.c in 0fa52f2 Aug 23, 2018

av+4 is not an atom but a term holding an atom. It isn't registered and doesn't need to be unregistered.

@JanWielemaker

This comment has been minimized.

Show comment
Hide comment
@JanWielemaker

JanWielemaker Aug 23, 2018

Member

But you probably do need an else clause that calls PL_domain_error()

Member

JanWielemaker commented on ssl4pl.c in 0fa52f2 Aug 23, 2018

But you probably do need an else clause that calls PL_domain_error()

@jamesnvc

This comment has been minimized.

Show comment
Hide comment
@jamesnvc

jamesnvc Aug 23, 2018

Contributor

I just pushed a change to find the string in the original list & did PL_unregister_atom -- is that how to "free" the string or do I not need to do anything? That seemed like the easiest way to me.

For the callback, I think the only return values that are understood are

The ALPN select callback cb, must return one of the following:

SSL_TLSEXT_ERR_OK
ALPN protocol selected.
SSL_TLSEXT_ERR_ALERT_FATAL
There was no overlap between the client's supplied list and the server configuration.
SSL_TLSEXT_ERR_NOACK
ALPN protocol not selected, e.g., because no ALPN protocols are configured for this connection.

So I'm not sure if there's a way to pass it back. I did the extended status so I could know to clear exceptions, because I was getting an error about "uncleared exception after leaving foreign frame", or something like that. I do currently see the exception at the top level if there is an exception inside the hook...is that reasonable?

As for exception on failure, I think failure is a reasonable to way indicate that there was no common protocol to be negotiated, so the callback returning SSL_TLSEXT_ERR_ALERT_FATAL seems like the most reasonable thing to do.

Contributor

jamesnvc commented Aug 23, 2018

I just pushed a change to find the string in the original list & did PL_unregister_atom -- is that how to "free" the string or do I not need to do anything? That seemed like the easiest way to me.

For the callback, I think the only return values that are understood are

The ALPN select callback cb, must return one of the following:

SSL_TLSEXT_ERR_OK
ALPN protocol selected.
SSL_TLSEXT_ERR_ALERT_FATAL
There was no overlap between the client's supplied list and the server configuration.
SSL_TLSEXT_ERR_NOACK
ALPN protocol not selected, e.g., because no ALPN protocols are configured for this connection.

So I'm not sure if there's a way to pass it back. I did the extended status so I could know to clear exceptions, because I was getting an error about "uncleared exception after leaving foreign frame", or something like that. I do currently see the exception at the top level if there is an exception inside the hook...is that reasonable?

As for exception on failure, I think failure is a reasonable to way indicate that there was no common protocol to be negotiated, so the callback returning SSL_TLSEXT_ERR_ALERT_FATAL seems like the most reasonable thing to do.

@JanWielemaker

This comment has been minimized.

Show comment
Hide comment
@JanWielemaker

JanWielemaker Aug 24, 2018

Member

The PL_Q_NORMAL should be PL_Q_PASS_EXCEPTION. normal is a bit misnomer for compatibility reasons and causes the debugger to be started if an exception happens. Pushed one final cleanup. I'm happy to (squash) merge into master now.

Alternatively you can use the branch/pull request as is while you work on the HTTP/2 stuff and you can extend this pull request if you encounter bugs or missing functionality. It is up to you. A merge now won't do any harm as this should not have negative impact on existing code.

Member

JanWielemaker commented Aug 24, 2018

The PL_Q_NORMAL should be PL_Q_PASS_EXCEPTION. normal is a bit misnomer for compatibility reasons and causes the debugger to be started if an exception happens. Pushed one final cleanup. I'm happy to (squash) merge into master now.

Alternatively you can use the branch/pull request as is while you work on the HTTP/2 stuff and you can extend this pull request if you encounter bugs or missing functionality. It is up to you. A merge now won't do any harm as this should not have negative impact on existing code.

@jamesnvc

This comment has been minimized.

Show comment
Hide comment
@jamesnvc

jamesnvc Aug 24, 2018

Contributor

Excellent, sounds good! Do you want me to squash the commits & force-push here, or can you do the squash when you merge?

I think it makes sense to keep the HTTP/2 stuff separate; I have stuff working-ish in another repo & this was the only change needed on the SSL side.

Contributor

jamesnvc commented Aug 24, 2018

Excellent, sounds good! Do you want me to squash the commits & force-push here, or can you do the squash when you merge?

I think it makes sense to keep the HTTP/2 stuff separate; I have stuff working-ish in another repo & this was the only change needed on the SSL side.

@JanWielemaker

This comment has been minimized.

Show comment
Hide comment
@JanWielemaker

JanWielemaker Aug 24, 2018

Member

Ok. I'll merge.

Member

JanWielemaker commented Aug 24, 2018

Ok. I'll merge.

@JanWielemaker

This comment has been minimized.

Show comment
Hide comment
@JanWielemaker

JanWielemaker Aug 24, 2018

Member

Merged. Please open a new pull request relative to the current master for possible subsequent changes. Thanks for the job and looking forward to the next step!

Member

JanWielemaker commented Aug 24, 2018

Merged. Please open a new pull request relative to the current master for possible subsequent changes. Thanks for the job and looking forward to the next step!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment