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.

Sign up for GitHub

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

Jump to bottom

Expose ssl function set session id context #751

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

Expose ssl function set session id context #751

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions include/pistache/endpoint.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -125,6 +125,8 @@ class Endpoint {
void useSSLAuth(std::string ca_file, std::string ca_path = "",
int (*cb)(int, void *) = NULL);

bool setSessionIdContext(const std::string &s_id_ctx);

bool isBound() const { return listener.isBound(); }

Port getPort() const { return listener.getPort(); }
Expand Down
1 change: 1 addition & 0 deletions include/pistache/listener.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -76,6 +76,7 @@ class Listener {
bool use_compression);
void setupSSLAuth(const std::string &ca_file, const std::string &ca_path,
int (*cb)(int, void *));
bool setSessionIdContext(const std::string &s_id_ctx);

private:
Address addr_;
Expand Down
4 changes: 4 additions & 0 deletions src/server/endpoint.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -105,6 +105,10 @@ Endpoint::requestLoad(const Tcp::Listener::Load &old) {
return listener.requestLoad(old);
}

bool Endpoint::setSessionIdContext(const std::string &s_id_ctx) {
return listener.setSessionIdContext(s_id_ctx);
}

Endpoint::Options Endpoint::options() { return Options(); }

} // namespace Http
Expand Down
7 changes: 7 additions & 0 deletions src/server/listener.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -444,6 +444,13 @@ void Listener::setupSSLAuth(const std::string &ca_file,
);
}

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 •
edited
Loading

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.

(unsigned int)s_id_ctx.length()) == 1;
}

void Listener::setupSSL(const std::string &cert_path,
const std::string &key_path, bool use_compression) {
SSL_load_error_strings();
Expand Down