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: add ssl.set_http_version API #147

Closed
wants to merge 5 commits into from
Closed

Conversation

vislee
Copy link

@vislee vislee commented Sep 23, 2017

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

Note: the unit test relying on the test-nginx/pull/68


ssl_certificate_by_lua_block {
local ssl = require "ngx.ssl";
ssl.set_http_version(2);
Copy link
Member

Choose a reason for hiding this comment

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

I think it's better to use the ssl.use_http2(true) and ssl.use_http2(false) here instead. We cannot really set other HTTP version numbers like 0.9, 1.0, and 1.1 here anyway.


--- config
ssl_certificate ../../cert/test.crt;
ssl_certificate_key ../../cert/test.key;
Copy link
Member

Choose a reason for hiding this comment

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

Better let the test scaffold automatically generate these under the hood. It's too much a burden for us test writers to write these in every HTTP/2 over TLS test cases ;)

@agentzh
Copy link
Member

agentzh commented Sep 23, 2017

@vislee I wonder if the protocol selection result in the TLS handshake can be remembered by subsequent new connections doing TLS session resumption (session ID or session ticket). I doubt it though. Have you tested the TLS session resumption case?

@lziest
Copy link

lziest commented Sep 23, 2017

I feel we need to really test session resumption, because in case of session resumption, the certificate callback will be skipped entirely. So I am a little skeptical about this PR right now.

@agentzh
Copy link
Member

agentzh commented Sep 23, 2017

@lziest Agreed. We need test cases for different kinds of session resumptions, SSL session IDs and TLS session tickets.

@vislee
Copy link
Author

vislee commented Sep 25, 2017

@agentzh @lziest Thanks for review and comments. I will try to modify it. @agentzh you can close this PR. Sorry!

@agentzh
Copy link
Member

agentzh commented Sep 25, 2017

@vislee You can close it yourself, BTW :) Closing this.

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.

3 participants