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

feature: ssl: support for TLS-PSK #1167

Open
wants to merge 30 commits into
base: master
Choose a base branch
from
Open

Conversation

vartiait
Copy link

@vartiait vartiait commented Oct 4, 2017

I hereby granted the copyright of the changes in this pull request
to the authors of this lua-nginx-module project.

Adds a support for TLS-PSK handshakes with following new directives to lua-nginx-module:

  • ssl_psk_by_lua_block { lua-script }
  • ssl_psk_by_lua_file
  • ssl_psk_identity_hint <tls_psk_identity_hint>
  • lua_ssl_psk_identity <tls_psk_identity>
  • lua_ssl_psk_key <tls_psk_key>

This is related to PR at lua-resty-core/pull/150

@agentzh
Copy link
Member

agentzh commented Oct 5, 2017

@vartiait Thanks for the contribution! I wonder, however, whether we can reuse the existing ssl_certificiate_by_lua* for this.

@ghedo @lziest Will you please have a look at this? Thanks!

@vartiait
Copy link
Author

vartiait commented Oct 6, 2017

@agentzh you're welcome!

I would like to keep the callbacks separate as OpenSSL callback for TLS-PSK (set with SSL_CTX_set_psk_server_callback) is not re-entrant whereas cert_cb is, but it's your call :)

By the way, have you noticed that some tests in t/129-ssl-socket.t seems to randomly fail due to a timeout when run by travis-ci?

@agentzh
Copy link
Member

agentzh commented Oct 6, 2017

@vartiait Seems like I cannot rebase your branch to the latest master? See

agentzh@fedora64 ~/git/lua-nginx-module (PR/1167)$ git rebase master
First, rewinding head to replay your work on top of it...
Applying: feature: TLS-PSK handshake control.
Using index info to reconstruct a base tree...
M	config
M	src/ngx_http_lua_common.h
M	src/ngx_http_lua_control.c
M	src/ngx_http_lua_headers.c
M	src/ngx_http_lua_module.c
M	src/ngx_http_lua_socket_tcp.c
M	src/ngx_http_lua_ssl.h
M	src/ngx_http_lua_util.h
.git/rebase-apply/patch:453: trailing whitespace.
 *
.git/rebase-apply/patch:1021: trailing whitespace.
ngx_http_lua_ffi_ssl_set_psk_key(ngx_http_request_t *r,
.git/rebase-apply/patch:1165: trailing whitespace.
 *
warning: 3 lines add whitespace errors.
Falling back to patching base and 3-way merge...
Auto-merging src/ngx_http_lua_util.h
Auto-merging src/ngx_http_lua_ssl.h
Auto-merging src/ngx_http_lua_socket_tcp.c
Auto-merging src/ngx_http_lua_module.c
CONFLICT (content): Merge conflict in src/ngx_http_lua_module.c
Auto-merging src/ngx_http_lua_control.c
Auto-merging src/ngx_http_lua_common.h
Auto-merging config
error: Failed to merge in the changes.
Patch failed at 0001 feature: TLS-PSK handshake control.
The copy of the patch that failed is found in: .git/rebase-apply/patch

When you have resolved this problem, run "git rebase --continue".
If you prefer to skip this patch, run "git rebase --skip" instead.
To check out the original branch and stop rebasing, run "git rebase --abort".

Please do not use "git merge" in your branch and keep your git history strictly linear. Thank you!

@agentzh
Copy link
Member

agentzh commented Oct 6, 2017

@vartiait Will you explain your use cases a bit? It seems that the use cases for this feature is very narrow.

@dndx
Copy link
Member

dndx commented Oct 7, 2017

It seems to me that TLS-PSK is not a commonly used technology at all. AFAIK none of the web browsers on the market supports it. Additionally, NGINX, Apache, Python, Golang all lacks support to it in the standard TLS implementation as well. Maintaining such an obscure feature in the long term is probably not worth the effort, at least not until TLS-PSK become more adopted on the Internet.

I would rather spend more time on migrating to OpenSSL 1.1 and add client side certificate support to cosocket. Since the later can do everything TLS-PSK can and is more secure and widely used.

@vartiait
Copy link
Author

vartiait commented Oct 7, 2017

@agentzh sorry for that, I rebased my branch now.

@vartiait
Copy link
Author

vartiait commented Oct 7, 2017

@agentzh @dndx Yes, TLS-PSK is mainly used with IoT (Internet of Things) devices which are performance-constrained with limited CPU power.

There has apparently lately been also development of TLS-PSK support on NGINX side,
but that approach is only for static PSK keys.

http://mailman.nginx.org/pipermail/nginx-devel/2017-August/010430.html
http://mailman.nginx.org/pipermail/nginx-devel/2017-September/010471.html

My case in on a telco/mobile side, where 3GPP has specified TLS-PSK as an authentication option
for CPU/power constrained devices in GAA/GBA architecture. Devices uses dynamic PSK keys
for authentication and that's the reason I wrote a support for ssl_psk_by_lua as
we are using OpenResty/NGINX as a platform to implement the GAA/GBA architecture.

3GPP specification TS 33.222 - Generic Authentication Architecture (GAA); Access to network application functions using Hypertext Transfer Protocol over Transport Layer Security (HTTPS):

http://www.etsi.org/deliver/etsi_ts/133200_133299/133222/14.00.00_60/ts_133222v140000p.pdf

The product where we are using OpenResty is called Radiator GBA/BSF Pack:

http://www.open.com.au/gba-bsf/

@agentzh
Copy link
Member

agentzh commented Oct 7, 2017

@vartiait That sounds interesting. Thanks for the info!

@agentzh
Copy link
Member

agentzh commented Oct 7, 2017

@vartiait I still think we should use ssl_certificate_by_lua* for that but we need a separate context constant so that we can disable those Lua APIs which may yield (like cosockets and sleep) in this context.

@vartiait
Copy link
Author

vartiait commented Oct 8, 2017

@agentzh okay, wouldn't that imply also a change to ngx_http_lua_ssl_cert_by_chunk to prevent it from creating a new thread with ngx_http_lua_new_thread when called by ngx_http_lua_ssl_psk_server_handler?

@vartiait
Copy link
Author

vartiait commented Oct 8, 2017

@agentzh ah, okay, never mind, I now read what ngx_http_lua_new_thread, lua_newthread, ngx_http_lua_run_thread and lua_resume do.

@vartiait
Copy link
Author

vartiait commented Oct 8, 2017

@agentzh @dndx I added a flag entered_psk_handler to ngx_http_lua_ssl_ctx_t which is set in TLS-PSK callback ngx_http_lua_ssl_psk_server_handler and when that flag is set, the context will be set to NGX_HTTP_LUA_CONTEXT_SSL_PSK (instead of NGX_HTTP_LUA_CONTEXT_SSL_CERT) in ngx_http_lua_ssl_cert_by_chunk. What do you think?

@vartiait
Copy link
Author

vartiait commented Oct 9, 2017

I removed separate ssl_psk_by_lua* handlers and updated TLS-PSK server callback to use ssl_certificate_by_lua*.

ssl_certificate_by_lua* handler will be called two times during TLS-PSK handshake, first time from ssl3_get_client_hello() (as with other ciphers) and the second time from ssl3_accept() when actually called to set the PSK key.

@vartiait
Copy link
Author

vartiait commented Oct 9, 2017

@agentzh How could I get an exit code or a return code from ssl_certificate_by_lua* in order to return ngx.ERROR to terminate the handshake?

I tried to copy ctx->exit_code to cctx->exit_code in ngx_http_lua_ssl_cert_by_chunk, but that change broke few test cases in t/139-ssl-cert-by.t so I reverted it.

Tuure Vartiainen added 14 commits October 14, 2017 13:42
* implemented the ssl_psk_by_lua_block and
  ssl_psk_by_lua_file directives for controlling the NGINX
  downstream TLS-PSK handshake dynamically with Lua.
* added pure C API for Lua library ngx.ssl in the
  lua-resty-core library for getting TLS-PSK client identity and
  setting TLS-PSK key.
* added ssl_psk_identity_hint directive for setting TLS-PSK identity hint
  for the NGINX downstream TLS-PSK handshake.
* added lua_ssl_psk_identity and lua_ssl_psk_key directives for
  setting the NGINX upstream TLS-PSK identity and key.
…entity_hint, lua_ssl_psk_identity and lua_ssl_psk_key directives.
@vartiait
Copy link
Author

@agentzh could you please comment the PR, I integrated TLS-PSK to ssl_certificate_by_lua*, are there some other tweaks needed?

@agentzh
Copy link
Member

agentzh commented Oct 24, 2017

@vartiait I'll have a look as soon as I can manage. Thanks!

@berndschoenbach
Copy link

What is the status here?

@mergify
Copy link

mergify bot commented Jun 26, 2020

This pull request is now in conflict :(

@mergify
Copy link

mergify bot commented Mar 20, 2023

This pull request is now in conflict :(

@mergify mergify bot added the conflict label Mar 20, 2023
@mergify mergify bot removed the conflict label May 10, 2023
@mergify
Copy link

mergify bot commented May 10, 2023

This pull request is now in conflict :(

@mergify mergify bot added the conflict label May 10, 2023
@mergify mergify bot removed the conflict label Sep 23, 2023
@mergify
Copy link

mergify bot commented Sep 23, 2023

This pull request is now in conflict :(

@mergify mergify bot added the conflict label Sep 23, 2023
@mergify mergify bot removed the conflict label Mar 6, 2024
Copy link

mergify bot commented Mar 6, 2024

This pull request is now in conflict :(

@mergify mergify bot added the conflict label Mar 6, 2024
@mergify mergify bot removed the conflict label May 27, 2024
Copy link

mergify bot commented May 27, 2024

This pull request is now in conflict :(

@mergify mergify bot added the conflict label May 27, 2024
@mergify mergify bot removed the conflict label Feb 13, 2025
Copy link

mergify bot commented Feb 13, 2025

This pull request is now in conflict :(

@mergify mergify bot added the conflict label Feb 13, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants