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: added new ffi function ngx_http_lua_ffi_ssl_ciphers. #1958

Closed
wants to merge 5 commits into from

Conversation

zhuizhuhaomeng
Copy link
Contributor

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

Copy link
Member

@doujiang24 doujiang24 left a comment

Choose a reason for hiding this comment

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

two more ideas:

  1. we'd better pass a memory buffer pointer from Lua to reduce memory allocation in the connection pool. But the current implementation is also acceptable to me.
  2. we can call ngx_ssl_get_ciphers in the ngx_http_lua_ffi_ssl_ciphers function if we decide to allocation memory from the connection pool to avoid coping code.

@zhuizhuhaomeng
Copy link
Contributor Author

two more ideas:

  1. we'd better pass a memory buffer pointer from Lua to reduce memory allocation in the connection pool. But the current implementation is also acceptable to me.
  2. we can call ngx_ssl_get_ciphers in the ngx_http_lua_ffi_ssl_ciphers function if we decide to allocation memory from the connection pool to avoid coping code.

I prefer the latter one because it is hard to decide the length of the buffer.

@bjne
Copy link

bjne commented Nov 1, 2021

The output format of ssl_get_chipers is imho ineffective for what it is useful for. What I would like to get out is
perhaps a table (table could be passed as input for reuse) or just a int array of ciphers shared between client
and server.

Also, SSL_get_shared_chipers, outputs a list of shared ciphers between client and server not respecting what
is actually enabled on the server. I would personally much rather see a filtered list, not including disabled ciphers

@zhuizhuhaomeng
Copy link
Contributor Author

The output format of ssl_get_chipers is imho ineffective for what it is useful for. What I would like to get out is perhaps a table (table could be passed as input for reuse) or just a int array of ciphers shared between client and server.

Can you be more specific about your application scenario?

@bjne
Copy link

bjne commented Nov 1, 2021

The output format of ssl_get_chipers is imho ineffective for what it is useful for. What I would like to get out is perhaps a table (table could be passed as input for reuse) or just a int array of ciphers shared between client and server.

Can you be more specific about your application scenario?

A common usecase here is probably to check if client supports EDCSA ciphers, and serve certs accordingly. Splitting/iterating/garbage collecting colon-separated strings here is less effective than returning uint32_t[]
of uint32_t SSL_CIPHER_get_protocol_id(const SSL_CIPHER *c);

Also, only the enabled ciphers (SSL_get1_supported_ciphers/SSL_get_client_ciphers) is more useful than also needing to know
what ciphers are actually enabled to make decision.

@zhuizhuhaomeng
Copy link
Contributor Author

The output format of ssl_get_chipers is imho ineffective for what it is useful for. What I would like to get out is perhaps a table (table could be passed as input for reuse) or just a int array of ciphers shared between client and server.

Can you be more specific about your application scenario?

A common usecase here is probably to check if client supports EDCSA ciphers, and serve certs accordingly. Splitting/iterating/garbage collecting colon-separated strings here is less effective than returning uint32_t[] of uint32_t SSL_CIPHER_get_protocol_id(const SSL_CIPHER *c);

Also, only the enabled ciphers (SSL_get1_supported_ciphers/SSL_get_client_ciphers) is more useful than also needing to know what ciphers are actually enabled to make decision.

Would you please submit a PR?

@bjne
Copy link

bjne commented Nov 2, 2021

The output format of ssl_get_chipers is imho ineffective for what it is useful for. What I would like to get out is perhaps a table (table could be passed as input for reuse) or just a int array of ciphers shared between client and server.

Can you be more specific about your application scenario?

A common usecase here is probably to check if client supports EDCSA ciphers, and serve certs accordingly. Splitting/iterating/garbage collecting colon-separated strings here is less effective than returning uint32_t[] of uint32_t SSL_CIPHER_get_protocol_id(const SSL_CIPHER *c);
Also, only the enabled ciphers (SSL_get1_supported_ciphers/SSL_get_client_ciphers) is more useful than also needing to know what ciphers are actually enabled to make decision.

Would you please submit a PR?

Yes, I can do that.

@bjne
Copy link

bjne commented Nov 3, 2021

The output format of ssl_get_chipers is imho ineffective for what it is useful for. What I would like to get out is perhaps a table (table could be passed as input for reuse) or just a int array of ciphers shared between client and server.

Can you be more specific about your application scenario?

A common usecase here is probably to check if client supports EDCSA ciphers, and serve certs accordingly. Splitting/iterating/garbage collecting colon-separated strings here is less effective than returning uint32_t[] of uint32_t SSL_CIPHER_get_protocol_id(const SSL_CIPHER *c);
Also, only the enabled ciphers (SSL_get1_supported_ciphers/SSL_get_client_ciphers) is more useful than also needing to know what ciphers are actually enabled to make decision.

Would you please submit a PR?

Yes, I can do that.

Here is the RFC pull request: #1962

@zhuizhuhaomeng zhuizhuhaomeng deleted the ciphers branch May 26, 2022 00:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants