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

support raw ssl cert/key/dhparam objects for initialization #59

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

Conversation

majuscule
Copy link
Contributor

As I mentioned in pr #58, this changeset adds backwards compatible support for raw cert/key/dhparam objects to be passed to Csocket. This will allow clients to continue to utilize Csockets after reading privileged files into memory and dropping privileges.

One thing I'm unsure of is how this should be incorporated into virtual bool SNIConfigureServer && static int __SNICallBack.

@jimloco
Copy link
Owner

jimloco commented Nov 25, 2015

Thanks for the work, but on reviewing there are some changes that need to happen before I can pull them in.

For example in many places the notation is incorrect ...
X509* m_sCertRaw;

  1. This is a pointer, and having the 's' prefix asserts that it is a string. We use 'p' for pointers so this would need to be 'm_pCertRaw'. I realize this isn't entirely consistent, (ie m_ssl_ctx), but at minimum we can't have pointers look like strings.
  2. Additionally, these new pointers aren't initialized in ::Init() to NULL, all pointers, integers, bools, etc need to be initialized.
  3. There are some syntax niggles, where code style doesn't really look close. The style we use is 'astyle --style=ansi -t4 --unpad-paren --pad-paren-in --keep-one-line-blocks', which is noted at the top of Csocket.h
  4. Why does this return a const reference for the member variables? They live as pointers in the class, and if they weren't set in by someone (they would be NULL then, see point 2), a const reference to a NULL pointer would be returned. So, lets just have these return a const pointer rather than a const reference.
  5. Lastly, this is a big enough change that I think a unit test is merited in examples

Some other examples of style niggles ...

  • CS_STRING DHParamFile = m_sDHParamFile.empty() ? m_sPemFile : m_sDHParamFile;
    That should be CS_STRING sDHParamFile
  • FILE *dhParamsFile = fopen( DHParamFile.c_str(), "r" );
    pdhParamsFile

@majuscule
Copy link
Contributor Author

Thank you for your comments and quick response! You are of course, completely correct re styling and notation. I'm not very familiar with C++ codebases, and was focused on getting the functionality working. I lost sight of these things during it, and I'm sorry for that. I've pushed two more commits, the first being purely style & naming, the second adding the NULL initialization and changing the get method signatures to return const pointers. If you'd like me to squash one or both of these commits before the merge, just let me know.

As for a unit test / example, would you prefer something standalone, or incorporated into ChatServer?

@jimloco
Copy link
Owner

jimloco commented Nov 26, 2015

No problem, jumping into code and going for it is a great way to figure things out, kudos.

  • I don't mind multiple commits, let me get through the holiday over here and I'll check it out then.
  • I think I would prefer a stand alone test we can run through valgrind, it can be very simple and minimalist, I just want to validate OpenSSL's behavior as the documentation on these things is often sparse.

For example, with SSL_CTX_use_certificate(), when you call that with a X509 argument, I believe what happens is the library keeps a reference count that is incremented with that call. Subsequently when the ssl_ctx object is destroyed it should decrement the counter and it should clean up fine with your final free(), I just want to see that with valgrind.

  • Finally, one last niggle, rather than using "Raw", how about the term 'Use' ... for example ...

X509 * m_pUseCert; //!< certificate object passed in by the user to use rather than the file
SetUseCert(...)
GetUseCert(...)

I'm not the best at naming conventions, but Raw looks a little out of place to me.

Once again, thanks for jumping in there!

sed -i \
  -e 's/m_pCertRaw/m_pUseCert/g' \
  -e 's/SetCertRaw/SetUseCert/g' \
  -e 's/GetCertRaw/GetUseCert/g' \
  -e 's/m_pKeyRaw/m_pUseKey/g'   \
  -e 's/SetKeyRaw/SetUseKey/g' \
  -e 's/GetKeyRaw/GetUseKey/g' \
  -e 's/m_pDHParamRaw/m_pUseDHParam/g' \
  -e 's/SetDHParamRaw/SetUseDHParam/g' \
  -e 's/GetDHParamRaw/GetUseDHParam/g' \
  Csocket.{cc,h}
@majuscule majuscule force-pushed the support-raw-ssl-objects branch from 9c1cbf5 to 86b7b2b Compare November 29, 2015 00:00
@majuscule
Copy link
Contributor Author

Hi again,
I've been working on this for the past few days. I've pushed a commit renaming the member variables and methods as you've suggested, and have written a unit test. It turns out, unsurprisingly, that a unit test was a good idea :-). My znc patch utilizing this changeset functions as expected, but the additional classes do not support the functionality I added.

I'm going to continue to work on this on my own, but I'm having trouble figuring out the constness of member variables. I was wondering if there's any time you'd be free to collaborate (irc?) and help me with my understanding. No worries either way, I'm committed to figuring this out.

FWIW, my email is [email protected].

Thanks!

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.

2 participants