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

Expose ssl function set session id context #751

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

david-s-svedberg
Copy link

Fix for Issue #750

bool Listener::setSessionIdContext(const std::string &s_id_ctx)
{
return SSL_CTX_set_session_id_context(GetSSLContext(ssl_ctx_),
(const unsigned char*)s_id_ctx.c_str(),
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please use C++ style casts. cppcheck and -Weffc++ will warn about C-style casts, and C++ casts can be type-checked by the compiler.

Please also add a unit test for this. You can another test case to the existing C++ tests.

Is it possible to read the session_id? if so, consider adding that method at the same time.

I'll happily merge this PR once the casting and testing are handled. Thank you very much for your contribution!

Copy link
Author

Choose a reason for hiding this comment

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

I've fixed the casts. I'll have to do some digging on the Get part, doesn't seem to be an equivalent Get function, but there seems to be other ways of getting it. I'll look through your existing tests and see if I can figure out how to test this in a meaningful way. If I get a Get function implemented then I can at least do a get-matches-set test, but I'll try to reproduce the original issue and create a test for that.

Copy link
Author

@david-s-svedberg david-s-svedberg May 9, 2020

Choose a reason for hiding this comment

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

Ok, so after a lot of digging around in openssl it seems that they really don't want you to poke around in the SSL_CTX as it is an "opaque structure". I can get around it by adding a callback to get all new sessions that are established and use const unsigned char *SSL_SESSION_get0_id_context(const SSL_SESSION *s, unsigned int *len) but it seems that this should be considered a write only option.

I could also just store the value passed into my set function and return that if present, but not sure that it will help anyone.

What do you think?

Copy link
Collaborator

Choose a reason for hiding this comment

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

As far as SSL goes, I don't know. I've never looked under the hood there, nor coded to their library. SSL support in Pistache was added by another contributor. My only concern with this PR is that we should not use C-style casts for getting anything, including the "(const char*)" and "(unsigned int)" casts that are in your PR. Please convert them to C++ style casts.

As for dealing with SSL; Do what you must. Please add a unit test - even if you cannot verify that the session ID was set correctly, verify that your call to set it does not crash.

Thank you very much for your contribution and patience.

Copy link
Author

Choose a reason for hiding this comment

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

Hi again, I've fixed the casts and I'll submit a simple unit test, frustrating to not being able to reproduce. I think I'll have time this weekend to fix it.

@dennisjenkins75
Copy link
Collaborator

Apparently, I started the review weeks ago, but failed to submit it. I apologize for the very long delay in providing feedback on this PR.

@david-s-svedberg
Copy link
Author

Apparently, I started the review weeks ago, but failed to submit it. I apologize for the very long delay in providing feedback on this PR.

no worries, switched over to my fork for the build in the meantime.

@dennisjenkins75
Copy link
Collaborator

David,
Would you still like this change merged into the "official pistache"? I will merge it if you add unit tests.

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